codeant-ai-for-open-source[bot] commented on PR #34629: URL: https://github.com/apache/superset/pull/34629#issuecomment-3628067883
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/34629/files#diff-f9df0ad83e0d67364d0d020a2a1a94192d1163ced61db53b7f8e3c80b7c21f08R86-R89'><strong>Possible runtime crash</strong></a><br>The code determines a base `languageCode` from `locale` (splitting on '-') but elsewhere the component still accesses `languages[locale]`. If `locale` contains a region (e.g. "en-US") and `languages` only includes base codes ("en", "fa"), `languages[locale]` is undefined and will cause a runtime error when rendering the flag icon. Add safe lookup / fallback for language entries.<br> - [ ] <a href='https://github.com/apache/superset/pull/34629/files#diff-db8cf7421e62c8943502bdb7d8a801928cca1eb23fbe11ed7426fd30463c4a2fR75-R78'><strong>Missing update after direction change</strong></a><br>The new `setDirection` wrapper calls `themeController.setDirection(direction)` but ThemeController likely does not notify listeners (no call to notify listeners in the controller). That means the ThemeProvider's local state and other listeners may not observe the direction change and the UI may not re-render to reflect RTL/LTR updates. Verify ThemeController.setDirection persists or notifies listeners, or ensure the provider forces a local update after calling the controller.<br> - [ ] <a href='https://github.com/apache/superset/pull/34629/files#diff-15db9c70c2026f4a43d9f0d25510d099a9254294493aa4334725661a000856d8R334-R342'><strong>Missing persistence / notification</strong></a><br>The new `setDirection` method mutates `globalTheme` but does not persist the direction to storage nor notify listeners. As a result direction changes may be lost across reloads and subscribers (UI components) may not be informed of the change. Consider persisting direction and calling `notifyListeners()` (and/or re-applying theme) so UI updates.<br> - [ ] <a href='https://github.com/apache/superset/pull/34629/files#diff-f9df0ad83e0d67364d0d020a2a1a94192d1163ced61db53b7f8e3c80b7c21f08R36-R39'><strong>API breaking change</strong></a><br>The `LanguagePicker` hook signature now requires a `setDirection` callback prop. Consumers that call this hook must be updated; otherwise this will cause TypeScript/ runtime errors. Either make the prop optional and guard its use, or update all callers. Validate all usages across the codebase.<br> - [ ] <a href='https://github.com/apache/superset/pull/34629/files#diff-449931f1bf449596ce5e3ab49828ee91c5fc3532cf191e4dbd44fff4ba779c61R404-R410'><strong>Context API mismatch</strong></a><br>`setDirection` was added to `ThemeContextType`. If the Theme provider implementation was not updated to provide this function, consumers will fail to compile or will get undefined at runtime. Verify that the provider implementation, default context value, and all consumers are updated accordingly.<br> - [ ] <a href='https://github.com/apache/superset/pull/34629/files#diff-449931f1bf449596ce5e3ab49828ee91c5fc3532cf191e4dbd44fff4ba779c61R129-R131'><strong>Breaking change</strong></a><br>The `direction` property was added as a required field to `SupersetSpecificTokens`. Making this field mandatory can break existing themes/consumers that don't include it. Consider making it optional or supplying a default value during Theme construction to avoid runtime/type breakage.<br> - [ ] <a href='https://github.com/apache/superset/pull/34629/files#diff-3507126924177fe4c98ebe988af789190f9147ac3b1462410ffb041e3953cdd9R199-R206'><strong>Dir attribute not reset</strong></a><br>When the theme direction flips back from 'rtl' to 'ltr' the code only sets the 'dir' and 'data-direction' attributes for the 'rtl' case and doesn't reset or update them for 'ltr'. This can leave the document in an incorrect state after toggling languages.<br> - [ ] <a href='https://github.com/apache/superset/pull/34629/files#diff-6f938c689926e7f99310b243cf8bdb7caacdfcde0b9bbdfc5e078834c0091062R88-R95'><strong>RTL scroll/cursor logic possibly targeting wrong element</strong></a><br>New logic sets `sizerRef.current.scrollLeft` depending on `theme.direction`. However, `sizerRef` is attached to the hidden sizing <span>, not the actual Input element. The span likely doesn't have scroll/caret properties (and the code already guards with setSelectionRange), so RTL scrolling/caret placement may not occur. This can break the intended RTL behavior (cursor movement and scroll) when editing. Consider using a dedicated ref for the real input element.<br> </td></tr> </table> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
