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>
- 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>
- 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.
- Updated package.json to use the correct API endpoint for schema generation. - Introduced optional 'branch' property in CurrentDocumentSettings type. - Enhanced setPluginData function to handle branch data. - Added BranchModel interface and new API endpoints for branch management in apiSchema. - Updated various components and hooks to support branch selection and data fetching. - Integrated BranchSelect component in ProjectSettings for user branch selection. - Implemented logic to handle default branch settings and display in the UI.
Contributor
WalkthroughAdds an ESLint v9 flat config and tooling upgrades; implements branching support across API schema, client types, hooks, and UI; refactors parent-node traversal and several tools for efficiency; introduces font caching for text formatting; applies memoization, debouncing, and query caching improvements; and applies widespread trailing-comma/formatting consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Plugin UI
participant Hook as useHasBranchingEnabled
participant APIKey as GET /v2/api-keys/current
participant APIProj as GET /v2/projects/{projectId}/branches
UI->>Hook: call useHasBranchingEnabled()
Hook->>APIKey: GET /v2/api-keys/current
APIKey-->>Hook: { projectId }
Hook->>APIProj: GET /v2/projects/{projectId}
APIProj-->>Hook: { ProjectModel (useBranching), branches }
Hook-->>UI: return hasBranchingEnabled & branches
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 docstrings
🧪 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/ui/components/Dialog/Dialog.tsx (1)
13-22:⚠️ Potential issue | 🟡 MinorCapture
ref.currentbefore cleanup to avoid stale reference.Adding
onCloseto the dependency array is correct and fixes the stale closure. However, the cleanup function readsref.currentat cleanup time, which may differ from when the listener was added (especially now that the effect re-runs whenonClosechanges). Capture the element in a local variable:Proposed fix
useEffect(() => { + const element = ref.current; const handler = (e: KeyboardEvent) => { if (e.code === "Escape") { onClose(); } }; - ref.current?.focus(); - ref.current?.addEventListener("keydown", handler); - return () => ref.current?.removeEventListener("keydown", handler); + element?.focus(); + element?.addEventListener("keydown", handler); + return () => element?.removeEventListener("keydown", handler); }, [onClose]);🤖 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 currently reads ref.current both when adding the keydown listener and again during cleanup, which can lead to a stale/different element being used; capture the element into a local constant (e.g., const el = ref.current) at the start of the useEffect, call el?.focus() and el?.addEventListener("keydown", handler), and use the same el in the cleanup to removeEventListener; keep the dependency on onClose and reference handler within the effect so the correct listener is removed.src/ui/components/Popover/Popover.tsx (1)
63-75:⚠️ Potential issue | 🟡 MinorCapture
popoverTrigger.currentbefore cleanup.Same issue as in
Dialog.tsx: the cleanup function readspopoverTrigger.currentat teardown time, which may benullor a different element if the ref changed. Capture it in a local variable:Proposed fix
useEffect(() => { - if (!popoverTrigger.current || !dropdownRef.current) { + const triggerElement = popoverTrigger.current; + if (!triggerElement || !dropdownRef.current) { return; } - popoverTrigger.current.addEventListener("click", open); + triggerElement.addEventListener("click", open); window.addEventListener("resize", computePositionAndSet); return () => { - popoverTrigger.current?.removeEventListener("click", open); + triggerElement.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 cleanup currently reads popoverTrigger.current at teardown which can be null or changed; inside the useEffect that registers listeners (useEffect referencing popoverTrigger, open, computePositionAndSet) capture the current element into a local const (e.g., const trigger = popoverTrigger.current) before adding listeners and then remove listeners from that captured trigger in the return cleanup; do the same for any other refs used (dropdownRef if you later use it) so the removeEventListener calls target the same element instance that addEventListener was called on.src/ui/hooks/useInterpolatedTranslation.ts (1)
149-158:⚠️ Potential issue | 🟠 MajorPreserve Tolgee markup when seeding
interpolatedTranslation.current.Line 153 strips tags before the equality check, so a translation like
<b>Hello</b>now collapses toHelloand Line 155 skips the ref update whennode.charactersis alreadyHello. That leavesinterpolatedTranslation.currentas plain text, which can drop rich-text markup for later consumers.💡 Suggested fix
useEffect(() => { - const newCharacters = getOrUpdateInterpolatedTranslation({ + const nextInterpolated = getOrUpdateInterpolatedTranslation({ paramsValues: paramsValues || {}, currentTranslation: newTranslation, - })?.replace(/<[^>]*>/g, ""); + }); + const newCharacters = nextInterpolated?.replace(/<[^>]*>/g, ""); - if (newCharacters != null && newCharacters !== previousCharacters) { - interpolatedTranslation.current = newCharacters; + if ( + nextInterpolated != null && + (newCharacters !== previousCharacters || + nextInterpolated !== previousCharacters) + ) { + interpolatedTranslation.current = nextInterpolated; } }, [rawTranslation, node?.isPlural]);🤖 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 - 158, The effect currently strips HTML tags from the candidate translation before comparing to previousCharacters, which causes the ref to drop Tolgee markup; instead, call getOrUpdateInterpolatedTranslation once into a variable (e.g., newWithMarkup), derive a stripped version only for the equality check (newStripped = newWithMarkup?.replace(/<[^>]*>/g, "")), and if newStripped is non-null and differs from previousCharacters then assign interpolatedTranslation.current = newWithMarkup so the markup is preserved; update references to useEffect, getOrUpdateInterpolatedTranslation, interpolatedTranslation.current, previousCharacters, rawTranslation and node?.isPlural accordingly.src/ui/views/StringDetails/StringDetails.tsx (1)
439-457:⚠️ Potential issue | 🔴 CriticalRemove unsupported
onChangeprop; useonValueInputoronInputinstead.The
TextboxNumericcomponent in@create-figma-plugin/uiv3.2.0 does not have anonChangeevent handler (removed as a breaking change in v3.0.1). Replace with one of:
onValueInput={(value: string) => { ... }}– receives the string input valueonNumericValueInput={(value: null | number) => { ... }}– receives parsed numeric valueonInput={(event: JSX.TargetedEvent) => { ... }}– raw event object if direct currentTarget.value access is neededThe current code will not execute because the prop doesn't exist.
🤖 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 prop is unsupported; replace it with onValueInput or onInput and adapt the handler: move the logic that sets setNeedsSubmission(true), updates currentNode.pluralParamValue and builds newNode (NodeInfo) into an onValueInput={(value: string) => { ... }} (or onInput to access event.currentTarget.value) and call setCurrentNode(newNode) only when translation != null; ensure you keep the same checks for empty string and preserve setting currentNode.pluralParamValue, setNeedsSubmission, and construction of newNode so the behavior of TextboxNumeric, currentNode, setCurrentNode, setNeedsSubmission and translation remains identical.src/ui/views/CreateCopy/CreateCopy.tsx (1)
55-57:⚠️ Potential issue | 🟠 MajorPass the active branch parameter to translation fetches.
The
allTranslationsLoadable.getDatacall is missing thebranchparameter. When a non-default branch is selected, CreateCopy will fetch translations from the wrong dataset.Pullalready passesbranchcorrectly; apply the same pattern here.Suggested fix
-import { useGlobalActions } from "@/ui/state/GlobalState"; +import { useGlobalActions, useGlobalState } from "@/ui/state/GlobalState"; ... export const CreateCopy: FunctionComponent = () => { const { setRoute } = useGlobalActions(); + const branch = useGlobalState((c) => c.config?.branch); ... const response = await allTranslationsLoadable.getData({ language, + branch: branch || undefined, });🤖 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 55 - 57, The call to allTranslationsLoadable.getData in CreateCopy.tsx is missing the branch parameter so translations for non-default branches are not fetched; update the call to pass the currently selected branch (e.g., the same branch variable/prop used by the Pull component) by adding branch: activeBranch (or the existing branch identifier in scope) to the getData argument object, ensuring CreateCopy fetches translations from the correct branch.
🧹 Nitpick comments (13)
src/utilities.ts (1)
28-57: Apply trailing commas consistently across all switch cases.Trailing commas were added to the
camelCase(line 35) andPascalCase(line 43) branches, but thesnake_case(line 50) andsnake_case_capitalized(line 56) branches are missing them. For consistency, consider adding trailing commas to all similar map operations.✨ Proposed fix for consistency
case "snake_case": return str .split(/\s+/) .filter(Boolean) - .map((word) => word.toLowerCase()) + .map((word) => word.toLowerCase(),) .join("_"); case "snake_case_capitalized": return str .split(/\s+/) .filter(Boolean) - .map((word) => word.charAt(0).toUpperCase() + word.slice(1)) + .map((word) => word.charAt(0).toUpperCase() + word.slice(1),) .join("_");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utilities.ts` around lines 28 - 57, The switch branches for "snake_case" and "snake_case_capitalized" are missing trailing commas on their .map(...) arrow function expressions, causing inconsistency with the "camelCase" and "PascalCase" branches; update the .map callbacks inside the "snake_case" and "snake_case_capitalized" cases to include trailing commas (same style as used in the "camelCase" and "PascalCase" branches) so all map operations are consistently formatted.src/ui/styles.css (1)
49-50: Consider addingwhite-space: nowrapfor reliable ellipsis behavior.
text-overflow: ellipsistypically requireswhite-space: nowrapto take effect. Without it, text may wrap instead of truncating with an ellipsis. Note that native<select>elements have limited and inconsistent cross-browser support for text-overflow styling.♻️ Suggested improvement
select { border: 1px solid var(--figma-color-border); overflow-x: hidden; text-overflow: ellipsis; + white-space: nowrap; border-radius: var(--border-radius-2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/styles.css` around lines 49 - 50, The current CSS rule that sets "overflow-x: hidden" and "text-overflow: ellipsis" needs "white-space: nowrap" added to ensure the ellipsis consistently appears; update the same selector (the rule containing overflow-x and text-overflow in src/ui/styles.css) to include white-space: nowrap, and if this targets native <select> elements note their text-overflow support is inconsistent across browsers and consider using a custom styled control if cross-browser truncation is required.src/ui/views/StringDetails/StringDetails.tsx (1)
460-464: Use strict inequality for placeholder filtering.On Line 463, prefer
!==over!=for clearer intent and to avoid coercion edge cases.Proposed diff
- p.name != tolgeeValue.parameter, + p.name !== tolgeeValue.parameter,🤖 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 460 - 464, The filter predicate in the StringDetails component uses loose inequality when comparing p.name and tolgeeValue.parameter; update the comparison in the lambda passed to .filter (the function referencing tolgeeValue.parameter and p.name) to use strict inequality !== instead of != to avoid coercion and make intent explicit.src/main/utils/nodeParents.ts (1)
52-80: Consider documenting the performance trade-off.The refactored helpers now each invoke
getAllParents, meaning multiple calls for the same node will traverse the parent chain multiple times. This is fine when callers usegetAllParentsdirectly (as in the updatedpreformatKey.ts), but could be a subtle performance regression for code that still calls multiple helpers separately.Consider adding a brief JSDoc note suggesting callers use
getAllParentsdirectly when they need multiple parent types for the same node.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/utils/nodeParents.ts` around lines 52 - 80, The helper accessors (getFrame, getArtboard, getComponent, getSection, getGroup) each call getAllParents individually which can re-traverse the parent chain and cause a performance regression when multiple helpers are used for the same node; update the JSDoc for these functions (and optionally the file header) to briefly note this trade-off and recommend callers call getAllParents(nodeId) once and reuse its returned .frame/.artboard/.component/.section/.group when they need multiple parent types for the same node.src/ui/components/NodeList/NodeRow.tsx (1)
21-30: Memoization added, but effectiveness depends on parent component.Wrapping
NodeRowwithmemo()is a reasonable optimization for virtualized lists. Note thatmemo()performs shallow prop comparison, so the memoization will only be effective if the parent component memoizes/stabilizes props likenode,action,keyComponent,nsComponent, andonClick. If these are recreated on each render, the memo wrapper won't prevent re-renders.🤖 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 - 30, NodeRow is wrapped with memo() but memoization will be ineffective if parent repeatedly recreates props; ensure stable references for props like node, action, keyComponent, nsComponent, onClick (or wrap them with useMemo/useCallback) in the parent so shallow comparison succeeds. Locate the NodeRow component (export const NodeRow = memo(...)) and update the calling code to memoize complex objects (node, action, keyComponent, nsComponent) with useMemo and callbacks (onClick) with useCallback, or alternatively pass primitive/stable identifiers instead of recreated objects to make memo() actually prevent re-renders.src/ui/views/Push/Changes.tsx (1)
27-36: Consider the performance impact ofminVisibleRows={Infinity}.Setting
minVisibleRowstoInfinitydisables row virtualization, meaning all items will render at once. This is fine for small lists, but if there are many new/changed keys, it could degrade performance. If large diffs are possible, consider keeping virtualization or adding a reasonable upper bound.🤖 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 - 36, The NodeList usage is disabling virtualization by setting minVisibleRows={Infinity}; change it to preserve virtualization for large lists by using a finite cap or conditional logic based on changes.newKeys.length (e.g., remove the prop or set minVisibleRows to Math.min(changes.newKeys.length, <reasonableLimit>) or only set Infinity for very small lists). Update the NodeList prop handling where NodeList is rendered and the items are derived from changes.newKeys to use this conditional/finite limit.src/ui/views/CopyView/CopyView.tsx (1)
45-49: Optional: Consider explicitly passinghasNamespacesEnabledfor consistency.The
getPullChangesfunction accepts an optionalhasNamespacesEnabledparameter (defaults totrue). WhilePull.tsxexplicitly passes this parameter, bothCopyView.tsxandCreateCopy.tsxrely on the default value. If conditional namespace behavior is needed here, pass the parameter explicitly; otherwise, the current approach is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/views/CopyView/CopyView.tsx` around lines 45 - 49, The call to getPullChanges currently relies on its default hasNamespacesEnabled value; explicitly pass the boolean to make behavior consistent with Pull.tsx (and match CreateCopy.tsx if intended). Update the invocation where changedNodes is computed (getPullChanges(connectedNodes.items, language!, translations)) to include the desired hasNamespacesEnabled argument (true or false) so the function signature getPullChanges(..., hasNamespacesEnabled) is explicit and consistent across CopyView.tsx and CreateCopy.tsx.src/main/endpoints/formatText.ts (1)
67-77: This cache still resets once per node update.The
cachedAvailableFontsvariable is scoped to eachformatTextinvocation, so batch updates viaupdateNodescallfigma.listAvailableFontsAsync()once per node instead of once total.Hoist the cache outside
formatTextand cache the promise to avoid redundant calls across the batch:Suggested fix
+let cachedAvailableFontsPromise: Promise<Font[]> | null = null; + export const formatText = async ({ nodeInfo, formatted, }: FormatTextEndpointArgs) => { ... - let cachedAvailableFonts: Font[] | null = null; const getAvailableFonts = async () => { - if (!cachedAvailableFonts) { - cachedAvailableFonts = await figma.listAvailableFontsAsync(); + if (!cachedAvailableFontsPromise) { + cachedAvailableFontsPromise = figma.listAvailableFontsAsync(); } - return cachedAvailableFonts; + return cachedAvailableFontsPromise; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/endpoints/formatText.ts` around lines 67 - 77, cachedAvailableFonts is currently declared inside formatText so each invocation (e.g., per node in updateNodes) recreates the cache and triggers figma.listAvailableFontsAsync repeatedly; move the cachedAvailableFonts variable and the getAvailableFonts/getFontsOfFamily helpers out of formatText to module scope and have cachedAvailableFonts store the Promise returned by figma.listAvailableFontsAsync (or the resolved array) so concurrent calls reuse the same in-flight request; update references in formatText to use the hoisted cachedAvailableFonts/getAvailableFonts/getFontsOfFamily accordingly.src/ui/views/Index/Index.tsx (1)
98-100: Empty dependency array may cause stale closures.
handleRefreshNamespacesusesnamespacesLoadable.refetchandallNodes.refetchbut has an empty dependency array. While react-query'srefetchfunctions are typically stable, explicitly including them would be safer and more maintainable.♻️ Consider adding dependencies
const handleRefreshNamespaces = useCallback(async () => { await Promise.all([namespacesLoadable.refetch(), allNodes.refetch()]); - }, []); + }, [namespacesLoadable.refetch, allNodes.refetch]);Alternatively, if the intent is to avoid re-renders, document why the empty array is safe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/views/Index/Index.tsx` around lines 98 - 100, handleRefreshNamespaces currently has an empty dependency array which can lead to stale closures for namespacesLoadable.refetch and allNodes.refetch; update the useCallback dependency array to include namespacesLoadable.refetch and allNodes.refetch (or namespacesLoadable and allNodes if those are the stable objects you track) so the callback always references the latest refetch functions, or add a short comment explaining why an empty array is deliberately safe if you confirm those refetch functions are stable.src/ui/hooks/useHasBranchingEnabled.ts (1)
13-24: Consider using a safer fallback forprojectId.When
apiKeyInfo.data?.projectIdis undefined, the fallback value0is passed to the path. While theenabledcondition prevents the query from running in this case, using a more explicit approach could improve clarity and safety against potential race conditions.♻️ Optional: Use type assertion or explicit typing
const projectQuery = useApiQuery({ url: "/v2/projects/{projectId}", method: "get", path: { - projectId: apiKeyInfo.data?.projectId ?? 0, + projectId: apiKeyInfo.data?.projectId as number, }, options: { cacheTime: 60000, staleTime: 60000, enabled: apiKeyInfo.data?.projectId !== undefined, }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/hooks/useHasBranchingEnabled.ts` around lines 13 - 24, The path fallback of 0 for projectId can hide bugs and is misleading because the query is disabled when apiKeyInfo.data?.projectId is undefined; update the useApiQuery call in useHasBranchingEnabled so the path.projectId is not forced to 0 but instead uses the actual possibly-undefined value (e.g., projectId: apiKeyInfo.data?.projectId) or an explicit typed assertion (e.g., as number | undefined) so you don't pass a sentinel 0; keep the existing enabled check (apiKeyInfo.data?.projectId !== undefined) to prevent execution when projectId is absent.src/ui/components/BranchSelect/BranchSelect.tsx (1)
44-51: Consider simplifying the nested setTimeout focus logic.The double-nested
setTimeout(100ms + 50ms) for focus restoration appears to be a workaround for timing issues after refresh. While it works, this approach is fragile and may behave inconsistently across different scenarios.Consider using a single
setTimeoutwith a slightly longer delay, or investigate ifrequestAnimationFramecould provide more reliable timing after the DOM updates.♻️ Suggested simplification
await onRefresh(); if (wasFocused && inputRef.current) { - setTimeout(() => { - inputRef.current?.focus(); - setTimeout(() => { - inputRef.current?.focus(); - }, 50); - }, 100); + // Allow DOM to settle after refresh before restoring focus + requestAnimationFrame(() => { + requestAnimationFrame(() => { + inputRef.current?.focus(); + }); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/BranchSelect/BranchSelect.tsx` around lines 44 - 51, The nested setTimeout calls used to restore focus (checking wasFocused and inputRef.current) are fragile; replace them with a single delayed focus attempt or use requestAnimationFrame to wait for DOM updates: when wasFocused is true, schedule one reliable focus call on inputRef.current (either via a single setTimeout with a slightly longer delay replacing the 100ms+50ms nesting, or by using requestAnimationFrame (or two successive requestAnimationFrame calls) to ensure rendering is complete before calling focus). Update the BranchSelect logic that references wasFocused, inputRef, and the current setTimeout nesting to perform one deterministic focus attempt.src/ui/views/Settings/ProjectSettings.tsx (1)
220-228: Verify edge case: effect may not trigger ifsettings.branchis an empty string.The condition
!settings.branchwould be true for bothundefinedand""(empty string). If a branch could legitimately have an empty name, this effect would incorrectly try to set the default branch. However, empty branch names are unlikely in practice.♻️ More explicit check for undefined
useEffect(() => { - if (settings && !settings.branch && branches.length > 0) { + if (settings && settings.branch === undefined && branches.length > 0) { const defaultBranch = branches.find((b) => b.isDefault)?.name; if (defaultBranch) { setSettings((prev) => ({ ...prev!, branch: defaultBranch })); } } }, [branches, settings?.branch]);🤖 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 220 - 228, The effect in ProjectSettings.tsx checks `!settings.branch`, which treats empty string as missing; change the condition to explicitly test for undefined (e.g., `settings.branch === undefined` or `settings.branch == null` if you also want to catch null) inside the useEffect so only truly absent branches trigger assigning the default; update the useEffect that references `branches`, `settings?.branch`, and calls `setSettings` to use this explicit check.src/ui/client/apiSchema.generated.ts (1)
1613-1621: Mirror the request-levelbranchshape on the deprecated resolvable import.
SingleStepImportResolvableRequestputsbranchin the right place, butImportKeysResolvableItemDtostill puts it on each key. That forces callers to repeat the same branch on every item and permits mixed-branch payloads in one request. Since this file is generated, I'd fix this in the upstream OpenAPI schema.♻️ Cleaner contract shape
ImportKeysResolvableDto: { + branch?: string; keys: components["schemas"]["ImportKeysResolvableItemDto"][]; }; ImportKeysResolvableItemDto: { name: string; - branch?: string; namespace?: string;Also applies to: 2871-2879
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/client/apiSchema.generated.ts` around lines 1613 - 1621, The generated DTO places branch on each ImportKeysResolvableItemDto which forces per-item repetition and allows mixed branches; update the OpenAPI schema (not just this file) so branch is defined at the request-level (the same shape as in SingleStepImportResolvableRequest) and remove the branch property from ImportKeysResolvableItemDto (also remove the duplicate branch occurrences around the other block noted near ImportKeysResolvableItemDto at ~2871-2879); regenerate the client so ImportKeysResolvableItemDto no longer contains branch and the request object carries the branch field consistently.
🤖 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 25-42: The config references the rule
"react-hooks/exhaustive-deps" but never registers the react-hooks plugin,
causing an unknown rule error; add the react-hooks plugin to the plugin imports
and register it in the plugins object (similar to tsPlugin/reactPlugin) — e.g.,
create or import a reactHooksPlugin variable and add reactHooks:
reactHooksPlugin to the plugins map in eslint.config.js so the
"react-hooks/exhaustive-deps" rule is valid (alternatively remove the
"react-hooks/exhaustive-deps" entry if you prefer not to use that plugin).
In `@package.json`:
- Around line 34-42: Update package.json to ensure ESLint v9 and Prettier v3 are
compatible: run the project linting/formatting locally and in CI using the
current "eslint" and "prettier" versions, fix or pin any incompatible ESLint
configs/plugins (e.g., "eslint-config-preact", "eslint-plugin-prettier",
"eslint-plugin-react") or Prettier integrations if they break, and adjust
package.json to pin working minor/patch ranges or add compatibility wrappers;
also confirm and add "scheduler" as a dependency to satisfy the peer requirement
of "use-context-selector" (>=0.19.0) if that package is present.
In `@src/main/endpoints/preformatKey.ts`:
- Around line 25-36: The replacement builds a RegExp from
separatorBeforePlaceholder which can contain regex metacharacters (from user
keyFormat) and thus may match incorrectly; in the function handling
preformatting (where originalFormat, placeholder, separatorBeforePlaceholder and
newFormat.replace are used) escape regex-special characters in
separatorBeforePlaceholder before passing it into new RegExp (use a utility or
inline escape that replaces characters like . * + ? ^ $ { } ( ) | [ ] \ / with
backslash-escaped versions) so the pattern matches the literal separator and
replacements behave correctly.
In `@src/ui/client/apiSchema.generated.ts`:
- Around line 5348-5353: The three generated operation types store_3,
executeComplexTagOperation_1, and getUsedNamespaces_1 currently declare
parameters? which allows undefined and breaks RequestParamsType expectations;
update each operation's type so parameters is required (parameters: { query?: {
... } }) while leaving the inner query optional, i.e. remove the optional marker
from the parameters wrapper for those three symbols so callers can safely access
parameters as assumed by the API client and types.ts.
In `@src/ui/hooks/useSelectedNodes.ts`:
- Around line 16-19: The debouncedRefetch callback references result.refetch but
does not include it in the useCallback dependency array, which can be incorrect
if a new query instance is created; update the debouncedRefetch useCallback to
include result.refetch in its dependency array (or explicitly document why it's
omitted) and ensure refetchTimerRef and any other values used inside
(refetchTimerRef, result.refetch) are accounted for so the memoized callback
stays correct; locate the debouncedRefetch declaration in useSelectedNodes.ts
and modify its dependency array accordingly.
In `@src/ui/views/CreateCopy/CreateCopy.tsx`:
- Around line 65-71: The loop currently calls copyPageMutation.mutate with
onSuccess: goToIndex which causes navigation as soon as the first mutation
succeeds; change to use copyPageMutation.mutateAsync (or the mutation's async
API) inside the loop to collect each mutation promise, then await
Promise.all(promises) before calling goToIndex so all copy operations complete
(or fail) before navigation; wrap the await in a try/catch to surface errors to
the user if any mutation rejects.
In `@src/ui/views/Pull/Pull.tsx`:
- Around line 38-43: The effect that calls computeDiff() currently only depends
on selectedNodes.data and lang, but computeDiff also reads branch and
hasNamespacesEnabled; update the effect dependency array to include branch and
hasNamespacesEnabled so the effect re-runs when the branch or namespace setting
changes (locate the useEffect that invokes computeDiff and add branch and
hasNamespacesEnabled to its dependency list alongside selectedNodes.data and
lang).
---
Outside diff comments:
In `@src/ui/components/Dialog/Dialog.tsx`:
- Around line 13-22: The effect currently reads ref.current both when adding the
keydown listener and again during cleanup, which can lead to a stale/different
element being used; capture the element into a local constant (e.g., const el =
ref.current) at the start of the useEffect, call el?.focus() and
el?.addEventListener("keydown", handler), and use the same el in the cleanup to
removeEventListener; keep the dependency on onClose and reference handler within
the effect so the correct listener is removed.
In `@src/ui/components/Popover/Popover.tsx`:
- Around line 63-75: The cleanup currently reads popoverTrigger.current at
teardown which can be null or changed; inside the useEffect that registers
listeners (useEffect referencing popoverTrigger, open, computePositionAndSet)
capture the current element into a local const (e.g., const trigger =
popoverTrigger.current) before adding listeners and then remove listeners from
that captured trigger in the return cleanup; do the same for any other refs used
(dropdownRef if you later use it) so the removeEventListener calls target the
same element instance that addEventListener was called on.
In `@src/ui/hooks/useInterpolatedTranslation.ts`:
- Around line 149-158: The effect currently strips HTML tags from the candidate
translation before comparing to previousCharacters, which causes the ref to drop
Tolgee markup; instead, call getOrUpdateInterpolatedTranslation once into a
variable (e.g., newWithMarkup), derive a stripped version only for the equality
check (newStripped = newWithMarkup?.replace(/<[^>]*>/g, "")), and if newStripped
is non-null and differs from previousCharacters then assign
interpolatedTranslation.current = newWithMarkup so the markup is preserved;
update references to useEffect, getOrUpdateInterpolatedTranslation,
interpolatedTranslation.current, previousCharacters, rawTranslation and
node?.isPlural accordingly.
In `@src/ui/views/CreateCopy/CreateCopy.tsx`:
- Around line 55-57: The call to allTranslationsLoadable.getData in
CreateCopy.tsx is missing the branch parameter so translations for non-default
branches are not fetched; update the call to pass the currently selected branch
(e.g., the same branch variable/prop used by the Pull component) by adding
branch: activeBranch (or the existing branch identifier in scope) to the getData
argument object, ensuring CreateCopy fetches translations from the correct
branch.
In `@src/ui/views/StringDetails/StringDetails.tsx`:
- Around line 439-457: TextboxNumeric's onChange prop is unsupported; replace it
with onValueInput or onInput and adapt the handler: move the logic that sets
setNeedsSubmission(true), updates currentNode.pluralParamValue and builds
newNode (NodeInfo) into an onValueInput={(value: string) => { ... }} (or onInput
to access event.currentTarget.value) and call setCurrentNode(newNode) only when
translation != null; ensure you keep the same checks for empty string and
preserve setting currentNode.pluralParamValue, setNeedsSubmission, and
construction of newNode so the behavior of TextboxNumeric, currentNode,
setCurrentNode, setNeedsSubmission and translation remains identical.
---
Nitpick comments:
In `@src/main/endpoints/formatText.ts`:
- Around line 67-77: cachedAvailableFonts is currently declared inside
formatText so each invocation (e.g., per node in updateNodes) recreates the
cache and triggers figma.listAvailableFontsAsync repeatedly; move the
cachedAvailableFonts variable and the getAvailableFonts/getFontsOfFamily helpers
out of formatText to module scope and have cachedAvailableFonts store the
Promise returned by figma.listAvailableFontsAsync (or the resolved array) so
concurrent calls reuse the same in-flight request; update references in
formatText to use the hoisted
cachedAvailableFonts/getAvailableFonts/getFontsOfFamily accordingly.
In `@src/main/utils/nodeParents.ts`:
- Around line 52-80: The helper accessors (getFrame, getArtboard, getComponent,
getSection, getGroup) each call getAllParents individually which can re-traverse
the parent chain and cause a performance regression when multiple helpers are
used for the same node; update the JSDoc for these functions (and optionally the
file header) to briefly note this trade-off and recommend callers call
getAllParents(nodeId) once and reuse its returned
.frame/.artboard/.component/.section/.group when they need multiple parent types
for the same node.
In `@src/ui/client/apiSchema.generated.ts`:
- Around line 1613-1621: The generated DTO places branch on each
ImportKeysResolvableItemDto which forces per-item repetition and allows mixed
branches; update the OpenAPI schema (not just this file) so branch is defined at
the request-level (the same shape as in SingleStepImportResolvableRequest) and
remove the branch property from ImportKeysResolvableItemDto (also remove the
duplicate branch occurrences around the other block noted near
ImportKeysResolvableItemDto at ~2871-2879); regenerate the client so
ImportKeysResolvableItemDto no longer contains branch and the request object
carries the branch field consistently.
In `@src/ui/components/BranchSelect/BranchSelect.tsx`:
- Around line 44-51: The nested setTimeout calls used to restore focus (checking
wasFocused and inputRef.current) are fragile; replace them with a single delayed
focus attempt or use requestAnimationFrame to wait for DOM updates: when
wasFocused is true, schedule one reliable focus call on inputRef.current (either
via a single setTimeout with a slightly longer delay replacing the 100ms+50ms
nesting, or by using requestAnimationFrame (or two successive
requestAnimationFrame calls) to ensure rendering is complete before calling
focus). Update the BranchSelect logic that references wasFocused, inputRef, and
the current setTimeout nesting to perform one deterministic focus attempt.
In `@src/ui/components/NodeList/NodeRow.tsx`:
- Around line 21-30: NodeRow is wrapped with memo() but memoization will be
ineffective if parent repeatedly recreates props; ensure stable references for
props like node, action, keyComponent, nsComponent, onClick (or wrap them with
useMemo/useCallback) in the parent so shallow comparison succeeds. Locate the
NodeRow component (export const NodeRow = memo(...)) and update the calling code
to memoize complex objects (node, action, keyComponent, nsComponent) with
useMemo and callbacks (onClick) with useCallback, or alternatively pass
primitive/stable identifiers instead of recreated objects to make memo()
actually prevent re-renders.
In `@src/ui/hooks/useHasBranchingEnabled.ts`:
- Around line 13-24: The path fallback of 0 for projectId can hide bugs and is
misleading because the query is disabled when apiKeyInfo.data?.projectId is
undefined; update the useApiQuery call in useHasBranchingEnabled so the
path.projectId is not forced to 0 but instead uses the actual possibly-undefined
value (e.g., projectId: apiKeyInfo.data?.projectId) or an explicit typed
assertion (e.g., as number | undefined) so you don't pass a sentinel 0; keep the
existing enabled check (apiKeyInfo.data?.projectId !== undefined) to prevent
execution when projectId is absent.
In `@src/ui/styles.css`:
- Around line 49-50: The current CSS rule that sets "overflow-x: hidden" and
"text-overflow: ellipsis" needs "white-space: nowrap" added to ensure the
ellipsis consistently appears; update the same selector (the rule containing
overflow-x and text-overflow in src/ui/styles.css) to include white-space:
nowrap, and if this targets native <select> elements note their text-overflow
support is inconsistent across browsers and consider using a custom styled
control if cross-browser truncation is required.
In `@src/ui/views/CopyView/CopyView.tsx`:
- Around line 45-49: The call to getPullChanges currently relies on its default
hasNamespacesEnabled value; explicitly pass the boolean to make behavior
consistent with Pull.tsx (and match CreateCopy.tsx if intended). Update the
invocation where changedNodes is computed (getPullChanges(connectedNodes.items,
language!, translations)) to include the desired hasNamespacesEnabled argument
(true or false) so the function signature getPullChanges(...,
hasNamespacesEnabled) is explicit and consistent across CopyView.tsx and
CreateCopy.tsx.
In `@src/ui/views/Index/Index.tsx`:
- Around line 98-100: handleRefreshNamespaces currently has an empty dependency
array which can lead to stale closures for namespacesLoadable.refetch and
allNodes.refetch; update the useCallback dependency array to include
namespacesLoadable.refetch and allNodes.refetch (or namespacesLoadable and
allNodes if those are the stable objects you track) so the callback always
references the latest refetch functions, or add a short comment explaining why
an empty array is deliberately safe if you confirm those refetch functions are
stable.
In `@src/ui/views/Push/Changes.tsx`:
- Around line 27-36: The NodeList usage is disabling virtualization by setting
minVisibleRows={Infinity}; change it to preserve virtualization for large lists
by using a finite cap or conditional logic based on changes.newKeys.length
(e.g., remove the prop or set minVisibleRows to Math.min(changes.newKeys.length,
<reasonableLimit>) or only set Infinity for very small lists). Update the
NodeList prop handling where NodeList is rendered and the items are derived from
changes.newKeys to use this conditional/finite limit.
In `@src/ui/views/Settings/ProjectSettings.tsx`:
- Around line 220-228: The effect in ProjectSettings.tsx checks
`!settings.branch`, which treats empty string as missing; change the condition
to explicitly test for undefined (e.g., `settings.branch === undefined` or
`settings.branch == null` if you also want to catch null) inside the useEffect
so only truly absent branches trigger assigning the default; update the
useEffect that references `branches`, `settings?.branch`, and calls
`setSettings` to use this explicit check.
In `@src/ui/views/StringDetails/StringDetails.tsx`:
- Around line 460-464: The filter predicate in the StringDetails component uses
loose inequality when comparing p.name and tolgeeValue.parameter; update the
comparison in the lambda passed to .filter (the function referencing
tolgeeValue.parameter and p.name) to use strict inequality !== instead of != to
avoid coercion and make intent explicit.
In `@src/utilities.ts`:
- Around line 28-57: The switch branches for "snake_case" and
"snake_case_capitalized" are missing trailing commas on their .map(...) arrow
function expressions, causing inconsistency with the "camelCase" and
"PascalCase" branches; update the .map callbacks inside the "snake_case" and
"snake_case_capitalized" cases to include trailing commas (same style as used in
the "camelCase" and "PascalCase" branches) so all map operations are
consistently formatted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f0e6ede-2bb3-4490-b352-f82dc4f6c909
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (82)
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/types.tssrc/ui/Plugin.tsxsrc/ui/client/apiSchema.custom.tssrc/ui/client/apiSchema.generated.tssrc/ui/client/client.tssrc/ui/client/decodeApiKey.tssrc/ui/client/types.tssrc/ui/client/useQueryApi.tssrc/ui/components/AutocompleteSelect/AutocompleteSelect.tsxsrc/ui/components/BranchSelect/BranchSelect.csssrc/ui/components/BranchSelect/BranchSelect.css.d.tssrc/ui/components/BranchSelect/BranchSelect.tsxsrc/ui/components/Dialog/Dialog.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/Popover/Popover.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/useHasBranchingEnabled.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/KeyInput.tsxsrc/ui/views/Index/ListItem.tsxsrc/ui/views/Pull/Pull.tsxsrc/ui/views/Push/Changes.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.tstsconfig.json
💤 Files with no reviewable changes (3)
- src/ui/components/Shared/HtmlText.tsx
- src/ui/hooks/useHasNamespacesEnabled.ts
- src/ui/views/Settings/PushSection.tsx
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ui/views/CreateCopy/CreateCopy.tsx (1)
48-70:⚠️ Potential issue | 🟠 MajorWrap async operations in try/catch to handle mutation failures.
The refactor to
mutateAsyncis correct, but if any mutation throws (e.g., Figma API errors incopyPageEndpoint, network issues), the error propagates uncaught,goToIndex()never executes, and the user is stuck on the page with no feedback.🛡️ Proposed fix to add error handling
const handleSubmit = async () => { + try { if (copyType === "keys") { await copyPageMutation.mutateAsync(undefined); } else { for (const language of selectedLanguages) { const response = await allTranslationsLoadable.getData({ language, }); const { changedNodes } = getPullChanges( connectedNodes.data?.items || [], language, response, ); await copyPageMutation.mutateAsync({ language, nodes: changedNodes, }); } } goToIndex(); + } catch (error) { + console.error("Failed to create page copy:", error); + // Consider using a toast/notification to inform the user + } };🤖 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 48 - 70, handleSubmit currently awaits mutateAsync calls without error handling; wrap the main async work in a try/catch (using handleSubmit) so any thrown error from copyPageMutation.mutateAsync or allTranslationsLoadable.getData is caught, call your existing error reporting (e.g., setError / toast / logger) in the catch with the caught error, and ensure goToIndex() is invoked appropriately (either in a finally block if you want to always navigate, or after successful completion in the try block) so the user is not left stuck; reference handleSubmit, copyPageMutation, allTranslationsLoadable.getData, getPullChanges, selectedLanguages, connectedNodes, and goToIndex when making the change.src/main/endpoints/preformatKey.ts (1)
27-44:⚠️ Potential issue | 🟡 MinorHandle empty placeholders at the start of
keyFormat.On Line 34, this branch only strips the separator before an empty placeholder. If the first token is missing,
{section}.{elementName}still formats to.titlebecause the.after{section}is untouched. Please cover the leading-placeholder case as well so optional prefixes don’t leave dangling separators.🤖 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 27 - 44, The branch that handles empty replaceString only removes the separator before a missing placeholder, causing a leading empty token to leave a dangling separator; update the logic in preformatKey to also detect when the placeholder is at the start (use originalFormat.indexOf(placeholder) === 0 or compute separatorAfterPlaceholder from originalFormat.slice(originalFormat.indexOf(placeholder) + placeholder.length, originalFormat.indexOf(placeholder) + placeholder.length + 1)) and remove the placeholder plus the following separator as well; adjust the RegExp logic (using escapeRegExp, placeholder and either separatorBeforePlaceholder or separatorAfterPlaceholder) so both "separator+placeholder" and "placeholder+separator" cases are stripped from newFormat when replaceString === "".
🧹 Nitpick comments (1)
src/ui/views/Pull/Pull.tsx (1)
106-112: Consider logging formatting errors in development.While the fallback to
n.charactersis reasonable, silently swallowing all formatting errors could mask issues with malformed ICU messages or parameter mismatches. A conditionalconsole.warnin development could help debugging.🤖 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 106 - 112, The catch block in Pull.tsx that currently swallows formatting errors should log the error in development to aid debugging: inside the catch(e) for the formatting attempt that returns "{ ...n, formatted: n.characters } as NodeInfo & { formatted: string }", add a conditional development-only warning (e.g. if (process.env.NODE_ENV !== 'production') console.warn(...)) that includes the caught error `e` and identifying info from `n` (such as `n.id` or `n.characters`) so you still return the fallback but surface formatting/ICU issues during development.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/endpoints/preformatKey.ts`:
- Around line 27-44: The branch that handles empty replaceString only removes
the separator before a missing placeholder, causing a leading empty token to
leave a dangling separator; update the logic in preformatKey to also detect when
the placeholder is at the start (use originalFormat.indexOf(placeholder) === 0
or compute separatorAfterPlaceholder from
originalFormat.slice(originalFormat.indexOf(placeholder) + placeholder.length,
originalFormat.indexOf(placeholder) + placeholder.length + 1)) and remove the
placeholder plus the following separator as well; adjust the RegExp logic (using
escapeRegExp, placeholder and either separatorBeforePlaceholder or
separatorAfterPlaceholder) so both "separator+placeholder" and
"placeholder+separator" cases are stripped from newFormat when replaceString ===
"".
In `@src/ui/views/CreateCopy/CreateCopy.tsx`:
- Around line 48-70: handleSubmit currently awaits mutateAsync calls without
error handling; wrap the main async work in a try/catch (using handleSubmit) so
any thrown error from copyPageMutation.mutateAsync or
allTranslationsLoadable.getData is caught, call your existing error reporting
(e.g., setError / toast / logger) in the catch with the caught error, and ensure
goToIndex() is invoked appropriately (either in a finally block if you want to
always navigate, or after successful completion in the try block) so the user is
not left stuck; reference handleSubmit, copyPageMutation,
allTranslationsLoadable.getData, getPullChanges, selectedLanguages,
connectedNodes, and goToIndex when making the change.
---
Nitpick comments:
In `@src/ui/views/Pull/Pull.tsx`:
- Around line 106-112: The catch block in Pull.tsx that currently swallows
formatting errors should log the error in development to aid debugging: inside
the catch(e) for the formatting attempt that returns "{ ...n, formatted:
n.characters } as NodeInfo & { formatted: string }", add a conditional
development-only warning (e.g. if (process.env.NODE_ENV !== 'production')
console.warn(...)) that includes the caught error `e` and identifying info from
`n` (such as `n.id` or `n.characters`) so you still return the fallback but
surface formatting/ICU issues during development.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 469d7fcc-4fa4-45d3-a811-14e96f89d0bd
📒 Files selected for processing (6)
eslint.config.jssrc/main/endpoints/preformatKey.tssrc/ui/components/Dialog/Dialog.tsxsrc/ui/hooks/useSelectedNodes.tssrc/ui/views/CreateCopy/CreateCopy.tsxsrc/ui/views/Pull/Pull.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ui/components/Dialog/Dialog.tsx
- src/ui/hooks/useSelectedNodes.ts
- eslint.config.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ui/hooks/useHasBranchingEnabled.ts (1)
27-38: Consider removing or gating the console.info for production.While the log is helpful during development/debugging, it will emit on every project load in production. Consider gating it behind an environment check or removing it entirely if the information is not essential for end users.
- useEffect(() => { - if (projectQuery.data) { - // eslint-disable-next-line no-console - console.info( - `[Tolgee] Branching feature: ${hasBranching ? "ENABLED" : "DISABLED — enable it in your Tolgee project settings (Settings → General → Branching)"}` - ); - } - }, [projectQuery.data, hasBranching]); + useEffect(() => { + if (projectQuery.data && process.env.NODE_ENV === 'development') { + // eslint-disable-next-line no-console + console.info( + `[Tolgee] Branching feature: ${hasBranching ? "ENABLED" : "DISABLED — enable it in your Tolgee project settings (Settings → General → Branching)"}` + ); + } + }, [projectQuery.data, hasBranching]);Alternatively, if this log is intentionally user-facing, consider keeping it as-is but be aware it runs on every mount where project data is available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/hooks/useHasBranchingEnabled.ts` around lines 27 - 38, The console.info in useHasBranchingEnabled (inside the useEffect that reads projectQuery.data and hasBranching) should be removed or gated for production; update the useEffect to either remove the console.info entirely or wrap the log call in an environment check (e.g., only run when process.env.NODE_ENV !== 'production' or another debug flag) or replace it with a debug/logger utility so it does not emit on every project load in production while still preserving the existing behavior during development.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ui/hooks/useHasBranchingEnabled.ts`:
- Around line 27-38: The console.info in useHasBranchingEnabled (inside the
useEffect that reads projectQuery.data and hasBranching) should be removed or
gated for production; update the useEffect to either remove the console.info
entirely or wrap the log call in an environment check (e.g., only run when
process.env.NODE_ENV !== 'production' or another debug flag) or replace it with
a debug/logger utility so it does not emit on every project load in production
while still preserving the existing behavior during development.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd89a154-d035-412c-88e0-dd1bf52fdc08
📒 Files selected for processing (2)
src/ui/hooks/useHasBranchingEnabled.tssrc/ui/views/Index/ListItem.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/views/Index/ListItem.tsx
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 by CodeRabbit
New Features
Performance Improvements
Bug Fixes
Chores