Conversation
optimize namespace selector optimize push behavior
WalkthroughRefactors namespace UI to AutocompleteSelect, adds namespace-aware hooks/queries, updates push/pull flows to respect namespace settings and improve error handling, adds parent-aware hidden-layer filtering, and introduces multiple UI, style, and type changes across client and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Push View
participant Hook as useHasNamespacesEnabled
participant Query as useQueryApi
participant API as API Server
participant Diff as getPushChanges
UI->>Hook: request hasNamespacesEnabled
Hook->>Query: GET /v2/api-keys/current
Query->>API: GET /v2/api-keys/current
API-->>Query: return apiKey (projectId)
Query-->>Hook: apiKey/projectId
Hook->>Query: GET /v2/projects/{projectId}
Query->>API: GET /v2/projects/{projectId}
API-->>Query: return project.useNamespaces
Query-->>Hook: hasNamespacesEnabled
Hook-->>UI: hasNamespacesEnabled
UI->>Diff: request translations & screenshots (namespaced if enabled)
Diff->>Query: fetch translations (conditional namespaces)
Query-->>Diff: translations
Diff->>Diff: compute KeyChanges (include ns if enabled)
Diff-->>UI: KeyChanges
UI->>UI: deduplicate nodes (O(n))
UI->>API: upload translations / screenshots / tags
API-->>UI: responses
UI->>Query: invalidate getConnectedNodes cache
UI-->>User: show success with pushed key counts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cypress/e2e/index.cy.ts (1)
32-53: Test logic is inconsistent after removingnamespacesDisabled.After removing
namespacesDisabled: truefrom the config (line 35), this test now has identical configuration to the first test ({ ...SIGNED_IN }), yet expects opposite behavior:
- Test 1 (line 8):
config: SIGNED_IN→ expects namespace selector to exist (lines 25-29)- Test 2 (line 35):
config: { ...SIGNED_IN }→ expects namespace selector to NOT exist (line 52)Without a config setting to hide the namespace selector, the expectation on line 52 will fail. Please either:
- Update the assertion to expect the selector to exist, or
- Add a config setting that actually hides the namespace selector, or
- Remove this test if it's no longer relevant after the namespace refactor.
🧹 Nitpick comments (5)
src/ui/hooks/useAllTranslations.ts (1)
39-43: Consider simplifying the empty namespace handling logic.When
namespacesis provided (truthy), line 33 assignsnsNamesdirectly fromnamespaces. This means ifnamespacesincludes"", thennsNameswill already include it, making the check on line 41 (!nsNames.includes("")) always true when needed. The defensive code appears correct but may be overly cautious.Consider this simplification if the intent is to handle only the explicit
namespacesparameter case:- // Ensure empty string is included if namespaces is provided and contains empty string - // This handles the default namespace case - if (namespaces && namespaces.includes("") && !nsNames.includes("")) { - nsNames.push(""); - } + // Ensure empty string is included when explicitly requested + if (namespaces?.includes("") && !nsNames.includes("")) { + nsNames.push(""); + }src/ui/components/NamespaceSelect/NamespaceSelect.tsx (1)
51-58: Consider simplifying the nested setTimeout pattern.The double
focus()calls with nested timeouts (100ms then 50ms) appear fragile and may not reliably restore focus across different browsers or system loads.Consider a single timeout with a reasonable delay, or use a more deterministic approach like listening for a state update callback:
if (wasFocused && inputRef.current) { - setTimeout(() => { - inputRef.current?.focus(); - // Small delay to ensure the options are updated - setTimeout(() => { - inputRef.current?.focus(); - }, 50); - }, 100); + // Allow time for options to update before refocusing + setTimeout(() => inputRef.current?.focus(), 150); }src/ui/client/useQueryApi.ts (1)
51-65: Consider extending MutationProps with customPaths for consistency.The
useApiQueryhook'sPathsgeneric now includescustomPaths(line 32), butMutationPropsstill defaults to onlypaths(line 54). If mutations need to support custom endpoints, update line 54 to match:export type MutationProps< Url extends keyof Paths, Method extends keyof Paths[Url], - Paths = paths + Paths = paths & customPaths > = {If mutations are intentionally restricted to generated paths only, this is acceptable as-is.
src/ui/components/AutocompleteSelect/AutocompleteSelect.tsx (1)
176-191: Consider reducing the blur delay timeout.The 200ms delay in
handleBlur(line 185) allows dropdown item clicks to register before closing, but it also delays the UI response. Consider:
- Reducing to 100-150ms if 200ms feels sluggish
- Using
onMouseDownfor dropdown items (which you already do) to avoid needing such a long delaysrc/ui/views/Push/Push.tsx (1)
82-92: Consider including empty namespace in the dependency key.The
requiredNamespacesKeyfilters out empty/falsy namespaces on line 88, but the actual namespace computation inside the effect (line 127) includes empty strings vianode.ns ?? "". While this works because the key is only for dependency tracking, aligning them would make the logic easier to follow.const uniqueNamespaces = Array.from( new Set( deduplicatedNodes - .map((node) => node.ns) - .filter((ns): ns is string => Boolean(ns)) + .map((node) => node.ns ?? "") ) );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
cypress/e2e/index.cy.ts(2 hunks)cypress/e2e/push.cy.ts(1 hunks)package.json(1 hunks)src/main/utils/nodeTools.ts(1 hunks)src/main/utils/settingsTools.ts(2 hunks)src/tools/getPushChanges.ts(3 hunks)src/types.ts(1 hunks)src/ui/client/apiSchema.custom.ts(1 hunks)src/ui/client/client.ts(3 hunks)src/ui/client/useQueryApi.ts(2 hunks)src/ui/components/ActionsBottom/ActionsBottom.css(1 hunks)src/ui/components/AutocompleteSelect/AutocompleteSelect.css(1 hunks)src/ui/components/AutocompleteSelect/AutocompleteSelect.css.d.ts(1 hunks)src/ui/components/AutocompleteSelect/AutocompleteSelect.tsx(1 hunks)src/ui/components/InfoTooltip/InfoTooltip.css(1 hunks)src/ui/components/NamespaceSelect/NamespaceSelect.css(1 hunks)src/ui/components/NamespaceSelect/NamespaceSelect.css.d.ts(1 hunks)src/ui/components/NamespaceSelect/NamespaceSelect.tsx(1 hunks)src/ui/components/NodeList/NodeRow.css(1 hunks)src/ui/components/NodeList/NodeRow.tsx(1 hunks)src/ui/hooks/useAllTranslations.ts(2 hunks)src/ui/hooks/useHasNamespacesEnabled.ts(1 hunks)src/ui/hooks/useSetNodesDataMutation.ts(1 hunks)src/ui/icons/SvgIcons.tsx(1 hunks)src/ui/styles.css(1 hunks)src/ui/views/Index/Index.css(0 hunks)src/ui/views/Index/Index.css.d.ts(0 hunks)src/ui/views/Index/Index.tsx(4 hunks)src/ui/views/Index/ListItem.tsx(3 hunks)src/ui/views/Pull/Pull.tsx(1 hunks)src/ui/views/Push/Push.tsx(10 hunks)src/ui/views/Settings/ProjectSettings.tsx(6 hunks)src/ui/views/Settings/PushSection.tsx(3 hunks)src/ui/views/Settings/Settings.tsx(1 hunks)src/ui/views/Settings/StringsEditor.css(1 hunks)src/ui/views/Settings/StringsSection.tsx(4 hunks)
💤 Files with no reviewable changes (2)
- src/ui/views/Index/Index.css.d.ts
- src/ui/views/Index/Index.css
🧰 Additional context used
🧬 Code graph analysis (14)
src/ui/hooks/useSetNodesDataMutation.ts (3)
src/main/endpoints/setNodesData.ts (2)
SetNodesDataProps(5-7)setNodesDataEndpoint(9-28)src/main/utils/delayed.ts (1)
delayed(1-6)src/main/endpoints/getConnectedNodes.ts (1)
getConnectedNodesEndpoint(9-22)
cypress/e2e/push.cy.ts (2)
src/web/urlConfig.ts (2)
createTestNode(82-93)SIGNED_IN(39-45)cypress/common/tools.ts (1)
visitWithState(5-9)
src/ui/hooks/useHasNamespacesEnabled.ts (1)
src/ui/client/useQueryApi.ts (1)
useApiQuery(29-49)
cypress/e2e/index.cy.ts (2)
src/web/urlConfig.ts (2)
createTestNode(82-93)SIGNED_IN(39-45)cypress/common/tools.ts (1)
visitWithState(5-9)
src/ui/client/useQueryApi.ts (2)
src/ui/client/apiSchema.generated.ts (1)
paths(7-895)src/ui/client/apiSchema.custom.ts (1)
customPaths(6-14)
src/ui/views/Settings/StringsSection.tsx (1)
src/ui/components/InfoTooltip/InfoTooltip.tsx (1)
InfoTooltip(15-33)
src/ui/views/Index/ListItem.tsx (1)
src/ui/components/NamespaceSelect/NamespaceSelect.tsx (1)
NamespaceSelect(15-96)
src/ui/views/Settings/PushSection.tsx (1)
src/ui/components/AutocompleteSelect/AutocompleteSelect.tsx (1)
AutocompleteSelect(39-327)
src/ui/client/apiSchema.custom.ts (1)
src/ui/client/apiSchema.generated.ts (2)
operations(3321-11264)components(899-3315)
src/ui/views/Push/Push.tsx (5)
src/tools/getPushChanges.ts (2)
KeyChanges(17-22)getPushChanges(24-105)src/types.ts (1)
NodeInfo(74-86)src/ui/hooks/useAllTranslations.ts (1)
useAllTranslations(12-108)src/ui/hooks/useHasNamespacesEnabled.ts (1)
useHasNamespacesEnabled(3-35)src/main/endpoints/getScreenshots.ts (1)
getScreenshotsEndpoint(9-54)
src/ui/views/Index/Index.tsx (2)
src/ui/hooks/useHasNamespacesEnabled.ts (1)
useHasNamespacesEnabled(3-35)src/ui/hooks/useConnectedNodes.ts (1)
useConnectedNodes(8-16)
src/tools/getPushChanges.ts (1)
src/tools/compareNs.ts (1)
compareNs(1-3)
src/ui/views/Settings/ProjectSettings.tsx (3)
src/ui/hooks/useHasNamespacesEnabled.ts (1)
useHasNamespacesEnabled(3-35)src/ui/components/InfoTooltip/InfoTooltip.tsx (1)
InfoTooltip(15-33)src/ui/client/decodeApiKey.ts (1)
getProjectIdFromApiKey(57-74)
src/ui/client/client.ts (2)
src/ui/client/apiSchema.generated.ts (1)
paths(7-895)src/ui/client/apiSchema.custom.ts (1)
customPaths(6-14)
🪛 GitHub Check: Static check 🪲
src/ui/views/Settings/Settings.tsx
[warning] 23-23:
'getProjectIdFromApiKey' is defined but never used
[warning] 15-15:
'useApiQuery' is defined but never used
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cypress
🔇 Additional comments (52)
src/ui/views/Pull/Pull.tsx (1)
56-59: LGTM! Clear error handling for image upload limits.The error handling logic is correct and the error message provides clear, actionable guidance to users.
src/ui/styles.css (1)
39-39: LGTM!The addition of the dark mode text color variable is appropriate. White text (
#ffffff) provides good contrast on dark backgrounds.src/ui/views/Settings/StringsEditor.css (1)
162-166: LGTM!The dark mode override for placeholder text improves readability. Using
--figma-color-text-primary(white) instead of grey text provides better contrast on dark backgrounds.cypress/e2e/index.cy.ts (1)
25-29: LGTM! Selector and assertion correctly updated.The migration from a select dropdown to an input-based namespace selector is properly reflected here. The functional expectation checking for an empty value is appropriate.
src/ui/components/ActionsBottom/ActionsBottom.css (1)
10-10: LGTM! Z-index adjustment aligns with UI layering updates.The z-index increase from 1 to 2 ensures proper stacking with other overlay components introduced in this PR.
src/ui/components/InfoTooltip/InfoTooltip.css (1)
6-6: LGTM! Z-index addition ensures proper tooltip layering.The added z-index coordinates with other UI stacking adjustments across the PR.
src/ui/hooks/useSetNodesDataMutation.ts (1)
9-20: LGTM! Query invalidation pattern ensures data freshness.The onSuccess handler properly invalidates the connected nodes query after mutation succeeds, ensuring the UI displays up-to-date data.
src/ui/components/NodeList/NodeRow.tsx (1)
88-88: LGTM! Improved label clarity.Changing "ns:" to "Namespace:" makes the UI more user-friendly and explicit.
src/types.ts (1)
118-118: LGTM! Settings type extension for hidden layer handling.The new
ignoreHiddenLayersIncludingChildrenproperty appropriately extends the GlobalSettings type for parent-chain visibility checks.src/ui/components/NodeList/NodeRow.css (1)
41-43: LGTM! CSS changes support improved namespace display.The overflow and text-overflow adjustments allow the full namespace text to be visible, complementing the "Namespace:" label change.
package.json (1)
9-9: LGTM! Schema generation script updated for new API structure.The updated endpoint URL and output path align with the UI-centric API schema organization introduced in this PR.
src/ui/hooks/useAllTranslations.ts (1)
101-103: LGTM!The
clearCache()method provides a clean way to reset translations data, which aligns with the broader namespace refresh functionality introduced in this PR.src/ui/components/NamespaceSelect/NamespaceSelect.css (1)
11-63: LGTM!The styling for the NamespaceSelect component is well-structured with:
- Proper positioning and layout for the input wrapper and refresh button
- Clear interaction states (hover, disabled)
- Smooth rotation animation for the refresh action
src/main/utils/settingsTools.ts (1)
76-76: LGTM!The new
ignoreHiddenLayersIncludingChildrensetting is properly integrated into the plugin data flow, being destructured from input and persisted to document settings. This aligns with the PR's objective to refine hidden layer detection logic.Also applies to: 95-95
src/ui/icons/SvgIcons.tsx (1)
187-201: LGTM!The new
Refreshicon component follows the established pattern of other icon components in this file, usingcurrentColorfor flexible theming and spreading props for maximum reusability.src/ui/views/Index/Index.tsx (3)
67-87: LGTM!The
allAvailableNamespacescomputation efficiently combines API and node-based namespaces with proper deduplication and sorting. The sorting logic correctly places empty string at the end while maintaining alphabetical order for other namespaces.
90-92: LGTM!The
handleRefreshNamespacesfunction properly refreshes both the API namespaces and connected nodes data sources usingPromise.all, ensuring the UI stays in sync with the latest namespace state.
241-246: LGTM!The updated
ListItemprops correctly wire up the namespace enablement flag, dynamically computed namespace list, and refresh callback, enabling the component to conditionally render namespace selection UI and trigger refreshes when needed.cypress/e2e/push.cy.ts (2)
82-115: LGTM!The test properly verifies that pushing new keys displays the correct count in the success message. The test flow is clear: setup test data → verify diff display → submit push → assert success count.
117-150: LGTM!The test properly verifies that pushing changed keys displays the correct count in the success message, following the same reliable pattern as the new keys test.
src/ui/components/AutocompleteSelect/AutocompleteSelect.css (1)
1-61: LGTM!The AutocompleteSelect styling is well-structured with:
- Proper positioning variants for dropdown direction (up/down)
- Clear visual hierarchy with headers and items
- Appropriate interaction states (hover, cursor)
- Themed colors using Figma CSS variables
src/tools/getPushChanges.ts (3)
38-50: LGTM on conditional namespace comparison.The logic correctly skips namespace comparison when
hasNamespacesEnabledis false, using short-circuit evaluation withcompareNs.
85-93: LGTM on KeyChangeValue construction.Setting
nstoundefinedwhen namespaces are disabled ensures consistent downstream handling.
24-30: LGTM on the function signature change.The parameter change to
hasNamespacesEnabledcorrectly controls namespace behavior in the push logic. The implementation properly guards namespace comparisons (line 42) and conditionally sets thensfield (line 87). All callers have been updated—the single caller in Push.tsx correctly passes the new parameter.src/main/utils/nodeTools.ts (1)
34-59: LGTM on the hidden layer visibility logic.The refactored logic correctly:
- Checks node's own visibility first
- Optionally traverses the parent chain when
ignoreHiddenLayersIncludingChildrenis enabled- Uses defensive programming with try-catch around parent traversal
One consideration: the catch block logs the error but allows the function to continue (potentially returning
true). This is a reasonable fallback—if we can't determine parent visibility, we include the node rather than silently exclude it.src/ui/client/client.ts (1)
124-127: LGTM on the extended Paths type.The intersection
paths & customPathscorrectly extends the API client to support both generated and custom endpoint definitions.src/ui/components/AutocompleteSelect/AutocompleteSelect.css.d.ts (1)
1-12: LGTM!Standard CSS module type declaration file with correct readonly properties and CommonJS-style export.
src/ui/components/NamespaceSelect/NamespaceSelect.tsx (1)
65-95: LGTM on the component structure.The integration with
AutocompleteSelectand conditional refresh button rendering is clean. TheisRefreshingstate correctly disables the button and applies the rotating animation.src/ui/components/NamespaceSelect/NamespaceSelect.css.d.ts (1)
1-12: LGTM!The CSS type definitions correctly expose the new class names needed for the enhanced NamespaceSelect UI (container, refresh button, animations, etc.).
src/ui/views/Settings/StringsSection.tsx (3)
82-98: LGTM!The help text clearly explains the behavior of the "Including all child texts" option and how it differs from the basic hidden layers setting.
139-142: LGTM!State management for
ignoreHiddenLayersIncludingChildrenfollows the existing patterns consistently, with proper initialization from config and handler to update both local state and config.Also applies to: 203-211
308-329: LGTM!The UI properly implements the nested checkbox pattern with appropriate indentation and conditional rendering. The InfoTooltip provides helpful context for users.
src/ui/views/Index/ListItem.tsx (3)
17-29: LGTM!The props interface correctly extends to support namespace enablement detection and refresh behavior. The function signature properly destructures the new props.
101-111: LGTM!The NamespaceSelect rendering is now properly gated by
hasNamespacesEnabledand theonRefreshcallback is correctly passed through for refresh functionality.
125-136: LGTM!The simplified InsertLink rendering based on
node.connectedis cleaner and maintains the same visual behavior (secondary color for unconnected state).src/ui/views/Settings/ProjectSettings.tsx (2)
136-142: LGTM!The namespace sorting logic correctly places empty strings at the end while maintaining alphabetical order for named namespaces.
20-77: LGTM!The dynamic help text functions properly generate project-specific documentation links. The dual functions (
namespaceHelpTextandnamespaceHelpTextSetUp) provide appropriate guidance for both enabled and disabled namespace states.src/ui/client/apiSchema.custom.ts (1)
1-68: LGTM!The custom API schema is well-structured and follows the same patterns as the generated schema. It properly:
- References existing
components["schemas"]types for consistency- Includes comprehensive response types for success and error cases
- Provides clear documentation comments
- Types the
projectIdparameter correctly as a numbersrc/ui/views/Settings/PushSection.tsx (2)
25-30: LGTM!The tag options are efficiently computed with
useMemoand properly sorted alphabetically for a better user experience.
88-97: LGTM!The
AutocompleteSelectintegration simplifies the tag management UI significantly. The configuration is appropriate with multi-select mode enabled and descriptive placeholders for existing and new tags.src/ui/components/AutocompleteSelect/AutocompleteSelect.tsx (5)
13-37: LGTM!The discriminated union for
Propscorrectly types single-select vs multi-select modes, ensuring type safety forvalueandonChangeat compile time.
73-99: LGTM!The display logic and sync effect correctly handle the single-select mode behavior:
- Shows the selected value when not focused
- Shows the input value when focused
- Syncs back to the selected value when focus is lost
101-129: LGTM!The filtering and validation logic correctly:
- Filters options based on input
- Excludes selected values in multi-select mode
- Detects exact matches
- Prevents duplicate additions
130-166: LGTM!Event handlers properly manage both single-select and multi-select flows with appropriate type guards and state updates.
193-326: LGTM!The rendering logic is comprehensive:
- Displays badges for multi-select values with remove functionality
- Provides accessible input with proper ARIA attributes
- Renders dropdown with dynamic positioning
- Highlights matching text segments
- Shows "Add" option for new values
- Handles empty states appropriately
src/ui/views/Push/Push.tsx (7)
44-50: LGTM!The new state variables (
isPushing,pushedKeysCount,screenshotCount) appropriately support the enhanced push progress tracking and success screen functionality.
56-68: Good performance optimization.The O(n²) to O(n) deduplication improvement using a Set is a solid change. The composite key correctly handles both key and namespace for uniqueness.
111-145: Well-structured effect with proper cancellation handling.The cancellation pattern using a
cancelledflag correctly prevents state updates after unmount or dependency changes. The early return whensuccessis true is a good optimization. The use of primitive keys as dependencies to avoid object reference issues is a valid pattern.
352-379: Good resilience pattern for non-critical operations.Wrapping tag addition in a separate try-catch allows the push to succeed even if tagging fails. The error message correctly indicates that translations were still updated.
412-419: Cache invalidation correctly preserves pushed count.Storing
keysPushedbefore clearing cache ensures the success screen displays accurate counts. The cache clear prevents stale translation data on subsequent operations.
474-488: Clean success screen implementation.The success UI correctly uses preserved counts and provides clear feedback to the user with the option to return to the main view.
489-490: LGTM!Including
isPushingin the loading condition correctly shows the loading state during the push operation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ui/views/Push/Push.tsx (1)
82-92: Consider extracting namespace computation to avoid duplication.This namespace extraction logic is duplicated in the useEffect (lines 125-129). While both implementations are correct, extracting it to a memoized value would improve maintainability.
Consider refactoring:
- const requiredNamespacesKey = useMemo(() => { - if (!hasNamespacesEnabled) return ""; - const uniqueNamespaces = Array.from( - new Set( - deduplicatedNodes - .map((node) => node.ns) - .filter((ns): ns is string => Boolean(ns)) - ) - ); - return uniqueNamespaces.sort().join(","); - }, [deduplicatedNodes, hasNamespacesEnabled]); + const requiredNamespaces = useMemo(() => { + if (!hasNamespacesEnabled || deduplicatedNodes.length === 0) return undefined; + return Array.from( + new Set(deduplicatedNodes.map((node) => node.ns ?? "")) + ); + }, [deduplicatedNodes, hasNamespacesEnabled]); + + const requiredNamespacesKey = useMemo(() => { + return requiredNamespaces ? requiredNamespaces.slice().sort().join(",") : ""; + }, [requiredNamespaces]);Then use
requiredNamespacesdirectly in the useEffect instead of recomputing it at lines 124-129.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/ui/views/Push/Push.tsx(10 hunks)
🔇 Additional comments (6)
src/ui/views/Push/Push.tsx (6)
56-68: LGTM! Efficient deduplication optimization.The Set-based approach correctly reduces complexity from O(n²) to O(n), and the composite key properly accounts for namespace differentiation.
111-193: LGTM! Well-implemented diff computation with cancellation.The centralized useEffect correctly handles async operations with proper cancellation to prevent stale state updates. The comprehensive dependency array ensures recomputation when relevant inputs change, and the early return when
successis true avoids unnecessary work.
398-398: LGTM! Stale closure issue resolved.The functional updater
setUploadedScreenshotCount((c) => c + 1)correctly addresses the stale closure bug mentioned in past review comments. This ensures the count increments properly across async iterations.
413-422: LGTM! Proper state management and cache invalidation.Storing the pushed keys count before clearing the cache and connecting nodes ensures accurate UI display, and clearing the translations cache ensures newly pushed keys are recognized on subsequent operations.
475-489: LGTM! Clear success feedback with accurate counts.The success screen provides immediate feedback using the stored
pushedKeysCountanduploadedScreenshotCount, avoiding the need to wait for diff recomputation. The conditional display based onuploadScreenshotsappropriately reflects the user's choice.
266-268: The ArrayBuffer cast is correct and safe.
screenshot.imageis typed asUint8Array(not Node.js Buffer), and accessing.bufferon aUint8Arrayreturns the underlyingArrayBuffer. The cast is redundant but harmless—the Blob constructor accepts bothArrayBufferand typed arrays likeUint8Arraydirectly.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/ui/views/Push/Push.tsx (1)
443-448: Reset uploadedScreenshotCount in handleRepeat.Similar to the past review comment about screenshot count, if a push partially completes (some screenshots uploaded) before failing,
uploadedScreenshotCountretains its value. On retry, the count will be incorrect since it starts from the previous partial value instead of zero.Apply this diff to reset the screenshot count:
const handleRepeat = () => { setError(false); setSuccess(false); setPushedKeysCount(0); + setUploadedScreenshotCount(0); setErrorMessage(undefined); };
🧹 Nitpick comments (1)
src/ui/views/Push/Push.tsx (1)
320-332: Optional: Simplify resolution for changedKeys.Since
changedKeysalways haveoldValue(by definition fromgetPushChanges), the conditional on line 328 will always evaluate to"OVERRIDE". Consider simplifying:translations: { [language]: { text: item.newValue, - resolution: item.oldValue ? "OVERRIDE" : "NEW", + resolution: "OVERRIDE", }, },This is defensive programming and not a bug, but simplification improves clarity.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cypress/e2e/index.cy.ts(1 hunks)cypress/e2e/push.cy.ts(2 hunks)src/ui/views/Push/Push.tsx(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/ui/views/Push/Push.tsx (5)
src/tools/getPushChanges.ts (2)
KeyChanges(17-22)getPushChanges(24-105)src/ui/hooks/useConnectedNodes.ts (1)
useConnectedNodes(8-16)src/types.ts (1)
NodeInfo(74-86)src/ui/hooks/useHasNamespacesEnabled.ts (1)
useHasNamespacesEnabled(3-35)src/main/endpoints/getScreenshots.ts (1)
getScreenshotsEndpoint(9-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cypress
🔇 Additional comments (18)
cypress/e2e/push.cy.ts (3)
82-115: LGTM! Well-structured test for new keys.The test follows existing patterns, includes clear comments, and appropriately verifies that the success message displays the correct count of pushed keys.
117-150: LGTM! Well-structured test for changed keys.The test follows existing patterns, includes clear comments, and appropriately verifies that the success message displays the correct count of pushed keys.
154-154: Verify test independence.The text change from "On the road" to "Changed text 2" aligns with the new test at line 120 that uses the same key. However, Cypress tests should be isolated and independent. Please confirm that:
- Tests don't share state or depend on execution order
- This change is for clarity/consistency rather than to fix test interdependence
If these tests are truly independent (as they should be), the original text "On the road" should work just as well as "Changed text 2".
Also applies to: 164-164
cypress/e2e/index.cy.ts (2)
25-29: LGTM! Selector and assertion updated correctly for AutocompleteSelect.The change from a
selectdropdown togeneral_namespace_select_inputand the functional assertion with the|| ""fallback appropriately handle the new AutocompleteSelect component behavior, where the value might be null or undefined.
54-54: The selectors in lines 54 and 84-87 are correct and should not be changed.general_node_list_row_namespacetargets a read-only display element in the NodeRow component for connected nodes, whilegeneral_namespace_select_inputtargets an editable AutocompleteSelect input in the NamespaceSelect component for unconnected nodes. These serve different UI purposes and intentionally use different selectors.src/ui/views/Push/Push.tsx (13)
32-32: LGTM! Namespace detection and progress tracking state additions.The new state variables (
isPushing,pushedKeysCount,uploadedScreenshotCount) and theuseHasNamespacesEnabledhook properly support the namespace-aware push flow and progress tracking requirements.Also applies to: 44-44, 47-47, 50-50, 71-71
56-68: Excellent performance optimization!The deduplication logic correctly reduces time complexity from O(n²) to O(n) using a Set, and properly includes namespace in the composite key to avoid conflating identically named keys from different namespaces.
73-110: Well-designed memoization strategy.The memoized keys (
nodesKey,requiredNamespacesKey,screenshotsKey) and extracted primitives fromtolgeeConfigeffectively stabilize the useEffect dependencies, preventing unnecessary recomputations while capturing all relevant changes including namespace updates.
111-193: Robust async effect with proper cancellation.The refactored diff computation properly handles:
- Namespace-aware translation loading
- Cancellation to prevent race conditions
- Comprehensive error handling with specific messages for invalid API keys
- Stable dependencies via memoized keys
The effect safely uses
deduplicatedNodeswithout including it in the deps array because changes tonodesare captured bynodesKey.
195-197: LGTM! Appropriate memoization.The
totalScreenshotCountis correctly memoized from the changes state and used consistently throughout the UI.
255-256: LGTM! Proper counter initialization.The counters are correctly reset before starting the push operation.
266-268: LGTM! Correct ArrayBuffer cast.The cast of
screenshot.image.buffer as ArrayBufferis correct;Uint8Array.bufferreturns anArrayBuffer, which is the proper type forBlobconstruction.
353-411: Excellent error handling with partial success support.The granular error handling allows the push to succeed even if tag application or screenshot updates fail, with clear user-facing messages. Line 398 correctly uses a functional state updater
setUploadedScreenshotCount((c) => c + 1)to avoid stale closure bugs in the async loop.
413-420: LGTM! Proper cache management.The key count is correctly stored before clearing the translation cache, ensuring the success screen displays accurate counts while the cache is refreshed for subsequent operations.
423-436: LGTM! Comprehensive error handling.The enhanced error messages provide specific, actionable feedback for common failure scenarios including invalid API keys, too many images, and import errors.
456-460: LGTM! Correct changesSize computation.Using
pushedKeysCountwhen displaying the success state ensures the UI shows the actual number of keys that were pushed, regardless of any subsequent diff recomputations.
475-489: LGTM! Improved success screen UX.The immediate success screen provides clear feedback with accurate counts, and the
successstate in the useEffect dependencies (line 192) correctly prevents unnecessary recomputation while showing the success message.
490-490: LGTM! Consistent UI updates.The UI consistently uses
totalScreenshotCountand properly includesisPushingin the loading condition, ensuring accurate progress display throughout the push flow.Also applies to: 507-520, 531-532
|
@JanCizmar please merge this, if it's fine :) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/ui/views/Push/Push.tsx (2)
147-152: Consider simplifying config reconstruction.The
configForDiffobject spreadstolgeeConfigand then re-assigns the same fields that are already present. You can passtolgeeConfigdirectly togetPushChangesor only override fields that differ from the source.🔎 Simplified approach
- // Reconstruct tolgeeConfig object for getPushChanges - const configForDiff = { - ...tolgeeConfig, - tags: tolgeeConfig?.tags, - updateScreenshots: tolgeeConfigUpdateScreenshots, - addTags: tolgeeConfigAddTags, - }; - setChanges( getPushChanges( deduplicatedNodes, translations, hasNamespacesEnabled, screenshots, - configForDiff + tolgeeConfig ) );
382-411: Functional state updater correctly prevents stale closure.Line 398 correctly uses
setUploadedScreenshotCount((c) => c + 1)to avoid the stale closure bug flagged in previous reviews. The individual try-catch blocks enable partial success when updating screenshots.Minor improvement: If multiple screenshots fail, only the last error message is preserved. Consider accumulating errors or showing a count of failures for better UX.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/ui/client/client.tssrc/ui/hooks/useHasNamespacesEnabled.tssrc/ui/views/Push/Push.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/hooks/useHasNamespacesEnabled.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cypress
🔇 Additional comments (18)
src/ui/client/client.ts (3)
6-6: LGTM: Custom paths import added.The import of
customPathsis used appropriately to extend the API path definitions.
140-146: Critical issue from past review has been fixed!The unreachable condition issue has been correctly resolved. The code now checks if
pathParams.projectIdis undefined (line 142) before assigning to it (line 145), making the conditional logic for URL rewriting work as intended.The current flow is correct:
- Extract projectId from API key
- If projectId exists and wasn't already provided in pathParams, add the placeholder to the URL
- Assign projectId to pathParams for placeholder replacement
Note: Line 145 will overwrite any explicit
pathParams.projectIdwith the value from the API key, which appears intentional as the API key is the source of truth.
127-127: No type conflicts found.The intersection type
paths & customPathsis safe. ThecustomPathsinterface adds the single endpoint"/v2/projects/{projectId}"which does not exist in the generatedpaths, so there are no overlapping keys and no type conflicts.src/ui/views/Push/Push.tsx (15)
56-68: LGTM! Excellent deduplication optimization.The refactoring from O(n²) to O(n) using a Set is a solid performance improvement. The composite key properly accounts for both key and namespace.
71-71: Good addition for namespace-aware logic.The
useHasNamespacesEnabledhook integrates well with the namespace-aware refactoring mentioned in the PR objectives.
74-109: Excellent memoization strategy.The stable keys (nodesKey, requiredNamespacesKey, screenshotsKey) and extracted primitive values from tolgeeConfig are well-designed optimizations that prevent unnecessary recomputation while maintaining correctness.
195-197: LGTM!The memoized
totalScreenshotCountis a good optimization for UI consistency.
255-257: LGTM!Proper initialization of push state counters before starting the push operation.
328-328: LGTM!The conditional resolution logic correctly distinguishes between overriding existing translations and creating new ones based on the presence of
oldValue.
353-380: Good partial-success error handling.The try-catch around tagging allows the push to succeed even if tagging fails, with a clear message to the user. This graceful degradation improves UX when non-critical operations fail.
414-420: LGTM! Proper cache invalidation.Storing the pushed key count before clearing the cache and then calling
clearCache()ensures newly pushed keys will be recognized in subsequent operations.
427-435: LGTM! Enhanced error handling.The specific error cases for
too_many_uploaded_imagesandimport_keys_errorprovide better user feedback than generic error messages.
443-449: LGTM! Past review feedback addressed.The reset logic now correctly includes both
setPushedKeysCount(0)andsetUploadedScreenshotCount(0), addressing the issues flagged in previous reviews about incomplete state resets on retry.
476-490: LGTM! Improved success screen UX.The success screen displays immediately after push completion with clear metrics (pushed keys count and uploaded screenshots count), providing good user feedback without waiting for data recomputation.
491-491: LGTM!Adding
isPushingto the loading condition correctly prevents user interaction during the push operation.
508-521: LGTM!Consistent use of
totalScreenshotCountthroughout the UI improves code clarity and maintainability.
532-544: LGTM!The finish button logic correctly handles the case where there are no changes and either no screenshots or screenshots are not being uploaded.
266-268: The type system already guarantees thatscreenshot.image.bufferis anArrayBuffersincescreenshot.imageis typed asUint8Array. The cast is redundant but does not cause issues.
…Changes component
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ui/views/Index/ListItem.tsx (2)
84-99: Double mutation on namespace change.When namespace changes, two mutations are triggered:
- The
useEffectat lines 84-90 fires whennamespacechangeshandleNsChangeat lines 94-96 also callssetNodesDataMutation.mutateThis causes redundant API calls. Since
handleNsChangealready performs the mutation, theuseEffectdependency onnamespacecreates a duplicate.🐛 Proposed fix: Remove the namespace useEffect
- useEffect(() => { - if (keyName && namespace !== node.ns) { - setNodesDataMutation.mutate({ - nodes: [{ ...node, key: keyName, ns: namespace }], - }); - } - }, [namespace]); - const handleNsChange = (node: NodeInfo) => (value: string) => { setNamespace(value); setNodesDataMutation.mutate({ nodes: [{ ...node, key: keyName, ns: value }], }); - node.key = keyName; - node.ns = value; };
97-98: Avoid direct mutation of props.Mutating
node.keyandnode.nsdirectly modifies the prop object, which is a React anti-pattern. This can cause unpredictable behavior since the parent component doesn't expect its data to be mutated. The mutation call should be sufficient to update the Figma nodes.🐛 Proposed fix
const handleNsChange = (node: NodeInfo) => (value: string) => { setNamespace(value); setNodesDataMutation.mutate({ nodes: [{ ...node, key: keyName, ns: value }], }); - node.key = keyName; - node.ns = value; };
🤖 Fix all issues with AI agents
In `@src/ui/hooks/useAllTranslations.ts`:
- Line 117: The error aggregation uses the wrong loadable: change the error
source from translationsLoadable.error to translationsBaseLoadable.error so
translation fetch failures are captured; update the code that computes the error
(where const error = namespacesLoadable.error || translationsLoadable.error) to
reference translationsBaseLoadable.error, ensuring this aligns with the loadData
implementation that calls translationsBaseLoadable.
🧹 Nitpick comments (2)
src/ui/hooks/useAllTranslations.ts (1)
17-20: Remove unusedtranslationsLoadablemutation.This mutation is defined but never invoked anywhere in the hook. Only
translationsBaseLoadableis used for fetching translations. Consider removing this dead code.♻️ Proposed removal
- const translationsLoadable = useApiMutation({ - url: "/v2/projects/translations/{languages}", - method: "get", - });Also update the error aggregation accordingly (see related comment).
src/ui/views/Index/ListItem.tsx (1)
80-82: SimplifyhandleKeyChange- unnecessary currying.
handleKeyChangereturns a function but the outer function takes no parameters, making the currying pattern unnecessary. This appears to be leftover from when it may have accepted anodeparameter.♻️ Proposed simplification
- const handleKeyChange = () => (value: string) => { + const handleKeyChange = (value: string) => { setKeyName(value); };And update the usage:
- <KeyInput value={keyName || ""} onChange={handleKeyChange()} /> + <KeyInput value={keyName || ""} onChange={handleKeyChange} />Also applies to: 106-106
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/ui/hooks/useAllTranslations.ts`:
- Around line 48-52: In useAllTranslations.ts the conditional block checking "if
(namespaces && namespaces.includes(\"\") && !nsNames.includes(\"\")) {
nsNames.push(\"\"); }" is dead code because nsNames is assigned from namespaces
earlier, so the second check can never be true; remove this unreachable
conditional entirely (delete the block referencing namespaces and nsNames) to
clean up logic and avoid confusion.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
UI/UX Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.