[WEB-2799] chore: global component and code refactor #6131
Merged
Conversation
This file contains 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
WalkthroughThe pull request introduces several enhancements across multiple components in the codebase. Key changes include the addition of new utility functions and a custom hook for managing local storage, new icon components, and a tabbed interface component. Additionally, existing components have been updated to support new properties for improved styling flexibility. The changes involve both the introduction of new files and modifications to existing exports, enhancing the overall functionality and usability of the UI package. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (6)
packages/ui/src/icons/comment-fill-icon.tsx (1)
5-14: Add accessibility attributes to SVG iconWhile the implementation is clean, consider adding accessibility attributes to improve screen reader support.
export const CommentFillIcon: React.FC<ISvgIcons> = ({ className = "text-current", ...rest }) => ( - <svg viewBox="0 0 24 24" className={`${className}`} xmlns="http://www.w3.org/2000/svg" {...rest}> + <svg + viewBox="0 0 24 24" + className={`${className}`} + xmlns="http://www.w3.org/2000/svg" + aria-hidden="true" + role="img" + {...rest} + >packages/ui/src/icons/epic-icon.tsx (1)
19-26: Optimize SVG pathsThe second path appears to be a duplicate of part of the first path. Consider combining them into a single path for better maintainability.
packages/ui/src/collapsible/collapsible-button.tsx (1)
11-12: Consider adding JSDoc comments for the new props.The new className props are well-typed and follow React conventions. Consider adding JSDoc comments to document their purpose and expected values.
Add documentation like this:
type Props = { + /** Additional class names to apply to the container */ className?: string; + /** Additional class names to apply to the title text */ titleClassName?: string; };packages/ui/src/progress/linear-progress-indicator.tsx (1)
9-9: Consider using consistent size increments.The size increments could be more consistent:
- sm: 8px (h-2)
- md: 12px (h-3)
- lg: 14px (h-3.5)
- xl: 14px (h-[14px])
Consider using standard Tailwind increments or following a consistent pattern.
- "h-[14px]": size === "xl", + "h-4": size === "xl", // 16px following the increment patternAlso applies to: 49-49
packages/ui/src/tabs/tabs.tsx (2)
8-26: Add documentation and improve type definitionsThe type definitions would benefit from:
- JSDoc documentation
- More descriptive prop names
- Validation for required props
Consider these improvements:
+/** + * Represents a single tab item in the tabbed interface + */ type TabItem = { key: string; + /** Optional icon component to display alongside the label */ icon?: FC<LucideProps>; + /** Optional label content */ label?: React.ReactNode; + /** Content to display when this tab is active */ content: React.ReactNode; + /** Whether this tab is disabled */ disabled?: boolean; }; +/** + * Props for the Tabs component + */ type TTabsProps = { + /** Array of tab items to display */ tabs: TabItem[]; + /** Unique key for storing selected tab in localStorage */ storageKey: string; + /** Optional actions to display next to the tabs */ actions?: React.ReactNode; + /** Optional key of the default selected tab */ defaultTab?: string; containerClassName?: string; tabListContainerClassName?: string; tabListClassName?: string; tabClassName?: string; tabPanelClassName?: string; };
45-94: Improve accessibility and performanceThe render implementation needs:
- Better accessibility
- Performance optimization
- Simplified className handling
Consider these improvements:
return ( + <ErrorBoundary fallback={<div>Error loading tabs</div>}> <div className="flex flex-col w-full h-full"> - <Tab.Group defaultIndex={currentTabIndex(storedValue ?? defaultTab)}> + <Tab.Group + defaultIndex={currentTabIndex(storedValue ?? effectiveDefaultTab)} + // Improve accessibility + as="div" + role="tablist" + aria-orientation="horizontal" + > <div className={cn("flex flex-col w-full h-full gap-2", containerClassName)}> <div className={cn("flex w-full items-center gap-4", tabListContainerClassName)}> <Tab.List as="div" className={cn( "flex w-full min-w-fit items-center justify-between gap-1.5 rounded-md text-sm p-0.5 bg-custom-background-80/60", tabListClassName )} > - {tabs.map((tab) => ( + {tabs.map((tab) => { + const Icon = tab.icon; + return ( <Tab className={({ selected }) => cn( `flex items-center justify-center p-1 min-w-fit w-full font-medium text-custom-text-100 outline-none focus:outline-none cursor-pointer transition-all rounded`, selected ? "bg-custom-background-100 text-custom-text-100 shadow-sm" : tab.disabled ? "text-custom-text-400 cursor-not-allowed" : "text-custom-text-400 hover:text-custom-text-300 hover:bg-custom-background-80/60", tabClassName ) } key={tab.key} onClick={() => { if (!tab.disabled) setValue(tab.key); }} disabled={tab.disabled} + aria-selected={selected} + role="tab" > - {tab.icon && <tab.icon className="size-4" />} + {Icon && <Icon className="size-4" aria-hidden="true" />} {tab.label} </Tab> - ))} + )})} </Tab.List> {actions && <div className="flex-grow">{actions}</div>} </div> <Tab.Panels as={Fragment}> {tabs.map((tab) => ( - <Tab.Panel key={tab.key} as="div" className={cn("relative outline-none", tabPanelClassName)}> + <Tab.Panel + key={tab.key} + as="div" + className={cn("relative outline-none", tabPanelClassName)} + role="tabpanel" + > {tab.content} </Tab.Panel> ))} </Tab.Panels> </div> </Tab.Group> </div> + </ErrorBoundary> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
packages/helpers/hooks/index.ts(1 hunks)packages/helpers/hooks/use-local-storage.tsx(1 hunks)packages/ui/src/collapsible/collapsible-button.tsx(2 hunks)packages/ui/src/icons/comment-fill-icon.tsx(1 hunks)packages/ui/src/icons/epic-icon.tsx(1 hunks)packages/ui/src/icons/index.ts(1 hunks)packages/ui/src/icons/info-fill-icon.tsx(1 hunks)packages/ui/src/index.ts(1 hunks)packages/ui/src/progress/linear-progress-indicator.tsx(3 hunks)packages/ui/src/tabs/index.ts(1 hunks)packages/ui/src/tabs/tabs.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/helpers/hooks/index.ts
- packages/ui/src/icons/index.ts
- packages/ui/src/tabs/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/helpers/hooks/use-local-storage.tsx
[error] 4-4: Invalid typeof comparison value: this expression is not a string literal
not a string literal
Unsafe fix: Compare the result of typeof with a valid type name
(lint/suspicious/useValidTypeof)
[error] 16-16: Invalid typeof comparison value: this expression is not a string literal
not a string literal
Unsafe fix: Compare the result of typeof with a valid type name
(lint/suspicious/useValidTypeof)
🔇 Additional comments (4)
packages/ui/src/index.ts (1)
31-31: LGTM! Export addition follows package conventions.
The new tabs module export maintains consistency with existing export patterns and preserves alphabetical ordering.
packages/ui/src/collapsible/collapsible-button.tsx (1)
26-31: LGTM! Proper implementation of className customization.
The className composition is well-implemented:
- Preserves base styles while allowing customization
- Uses cn helper correctly for class merging
- Maintains existing functionality
Also applies to: 41-41
packages/ui/src/progress/linear-progress-indicator.tsx (2)
52-54: LGTM! Clean implementation of container customization.
The container class composition is well-implemented:
- Preserves base styles
- Uses cn helper correctly
- Maintains existing background and rounded corners
38-38: Verify the impact of new bar class names.
The addition of rounded corners to individual bars might affect existing implementations.
✅ Verification successful
Let me analyze the usage of LinearProgressIndicator to verify if the new bar class names would have any impact.
From the search results, I can see that LinearProgressIndicator is used in three locations:
- module-card-item.tsx
- cycles/active-cycle/progress.tsx
- The component definition itself
In both usage locations, they don't override the bar styling through any custom className props. Let me verify if there's a barClassName prop being passed anywhere.
Let me try a different approach to search for barClassName usage.
The new bar class names are safe to implement
The rounded corners addition through first:rounded-l-sm last:rounded-r-sm classes is safe as:
- The
barClassNameprop is optional and defaults to an empty string - No existing implementations are passing custom
barClassNameprops - The new classes only affect the corners and won't interfere with other styling properties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing usage of LinearProgressIndicator to verify styling impact
rg -l "LinearProgressIndicator" --type tsx --type jsx | xargs rg -A 5 "LinearProgressIndicator"
Length of output: 2980
Script:
#!/bin/bash
# Search for any usage of barClassName prop
rg "barClassName" --type tsx --type jsx
Length of output: 67
Script:
#!/bin/bash
# Search for barClassName without file type restriction
rg "barClassName"
Length of output: 329
Comment on lines
+5
to
+14
| export const InfoFillIcon: React.FC<ISvgIcons> = ({ className = "text-current", ...rest }) => ( | ||
| <svg viewBox="0 0 24 24" className={`${className}`} xmlns="http://www.w3.org/2000/svg" {...rest}> | ||
| <path | ||
| fillRule="evenodd" | ||
| clipRule="evenodd" | ||
| d="M2.25 12C2.25 6.615 6.615 2.25 12 2.25C17.385 2.25 21.75 6.615 21.75 12C21.75 17.385 17.385 21.75 12 21.75C6.615 21.75 2.25 17.385 2.25 12ZM10.956 10.558C12.102 9.985 13.393 11.021 13.082 12.264L12.373 15.1L12.415 15.08C12.5912 15.0025 12.7905 14.9958 12.9715 15.0612C13.1526 15.1265 13.3016 15.259 13.3877 15.4312C13.4737 15.6033 13.4903 15.802 13.434 15.9861C13.3777 16.1702 13.2527 16.3255 13.085 16.42L13.045 16.442C11.898 17.015 10.607 15.979 10.918 14.736L11.628 11.9L11.586 11.92C11.4975 11.9692 11.4 11.9999 11.2994 12.0104C11.1987 12.0209 11.097 12.0109 11.0003 11.981C10.9036 11.9511 10.8139 11.902 10.7367 11.8366C10.6595 11.7711 10.5964 11.6907 10.551 11.6002C10.5057 11.5098 10.4792 11.411 10.4731 11.31C10.4669 11.209 10.4813 11.1078 10.5153 11.0124C10.5493 10.9171 10.6022 10.8297 10.6709 10.7553C10.7396 10.681 10.8226 10.6214 10.915 10.58L10.956 10.558ZM12 9C12.1989 9 12.3897 8.92098 12.5303 8.78033C12.671 8.63968 12.75 8.44891 12.75 8.25C12.75 8.05109 12.671 7.86032 12.5303 7.71967C12.3897 7.57902 12.1989 7.5 12 7.5C11.8011 7.5 11.6103 7.57902 11.4697 7.71967C11.329 7.86032 11.25 8.05109 11.25 8.25C11.25 8.44891 11.329 8.63968 11.4697 8.78033C11.6103 8.92098 11.8011 9 12 9Z" | ||
| fill="currentColor" | ||
| /> | ||
| </svg> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider icon component abstraction and add accessibility
- Add accessibility attributes as suggested for CommentFillIcon.
- Consider creating a base icon component to reduce duplication across icon components.
export const InfoFillIcon: React.FC<ISvgIcons> = ({ className = "text-current", ...rest }) => (
- <svg viewBox="0 0 24 24" className={`${className}`} xmlns="http://www.w3.org/2000/svg" {...rest}>
+ <svg
+ viewBox="0 0 24 24"
+ className={`${className}`}
+ xmlns="http://www.w3.org/2000/svg"
+ aria-hidden="true"
+ role="img"
+ {...rest}
+ >Consider creating a base icon component:
type BaseIconProps = ISvgIcons & {
children: React.ReactNode;
viewBox?: string;
};
const BaseIcon: React.FC<BaseIconProps> = ({
className = "text-current",
viewBox = "0 0 24 24",
children,
...rest
}) => (
<svg
viewBox={viewBox}
className={className}
xmlns="http://www.w3.org/2000/svg"
aria-hidden="true"
role="img"
{...rest}
>
{children}
</svg>
);
Comment on lines
+3
to
+8
| export type Props = { | ||
| className?: string; | ||
| width?: string | number; | ||
| height?: string | number; | ||
| color?: string; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize props interface with other icons
The component uses a custom Props type while other icons use ISvgIcons. Also, the color prop is defined but unused.
-export type Props = {
- className?: string;
- width?: string | number;
- height?: string | number;
- color?: string;
-};
+import { ISvgIcons } from "./type";Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+10
to
+28
| export const EpicIcon: React.FC<Props> = ({ width = "16", height = "16", className }) => ( | ||
| <svg | ||
| width={width} | ||
| height={height} | ||
| className={className} | ||
| viewBox="0 0 16 16" | ||
| fill="none" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| > | ||
| <path | ||
| d="M0.900146 9.33203V12.0142C0.900146 12.3736 1.17392 12.6654 1.51126 12.6654H14.9557C15.1178 12.6654 15.2732 12.5968 15.3878 12.4746C15.5024 12.3525 15.5668 12.1869 15.5668 12.0142V10.3299L13.375 7.99523C13.1458 7.75134 12.8351 7.61436 12.5113 7.61436C12.1874 7.61436 11.8767 7.75134 11.6476 7.99523L10.1257 9.35919L10.2534 9.56204L11.7209 9.60056C11.7809 9.66017 11.8291 9.73206 11.8625 9.81194C11.8959 9.89181 11.9138 9.97804 11.9153 10.0655C11.9167 10.1529 11.9017 10.2397 11.8709 10.3208C11.8402 10.4019 11.7944 10.4756 11.7364 10.5374C11.6784 10.5992 11.6092 10.648 11.5332 10.6807C11.4571 10.7135 11.3756 10.7296 11.2935 10.728C11.2114 10.7265 11.1305 10.7073 11.0556 10.6717C10.9806 10.6362 10.9131 10.5848 10.8572 10.5209L10.2534 9.56204L6.60385 3.76614C6.37468 3.52226 6.11293 3.33203 5.78904 3.33203C5.46515 3.33203 5.20339 3.52226 4.97422 3.76614L0.900146 9.33203Z" | ||
| fill="currentColor" | ||
| /> | ||
| <path | ||
| d="M11.7209 9.60056L10.2534 9.56204L10.8572 10.5209C10.9131 10.5848 10.9806 10.6362 11.0556 10.6717C11.1305 10.7073 11.2114 10.7265 11.2935 10.728C11.3756 10.7296 11.4571 10.7135 11.5332 10.6807C11.6092 10.648 11.6784 10.5992 11.7364 10.5374C11.7944 10.4756 11.8402 10.4019 11.8709 10.3208C11.9017 10.2397 11.9167 10.1529 11.9153 10.0655C11.9138 9.97804 11.8959 9.89181 11.8625 9.81194C11.8291 9.73206 11.7809 9.66017 11.7209 9.60056Z" | ||
| fill="currentColor" | ||
| /> | ||
| </svg> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize SVG structure and add accessibility
- Use ISvgIcons interface for consistency
- Remove redundant fill="none" since paths use currentColor
- Add accessibility attributes
-export const EpicIcon: React.FC<Props> = ({ width = "16", height = "16", className }) => (
+export const EpicIcon: React.FC<ISvgIcons> = ({ className = "text-current", ...rest }) => (
<svg
- width={width}
- height={height}
className={className}
viewBox="0 0 16 16"
- fill="none"
xmlns="http://www.w3.org/2000/svg"
+ aria-hidden="true"
+ role="img"
+ {...rest}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const EpicIcon: React.FC<Props> = ({ width = "16", height = "16", className }) => ( | |
| <svg | |
| width={width} | |
| height={height} | |
| className={className} | |
| viewBox="0 0 16 16" | |
| fill="none" | |
| xmlns="http://www.w3.org/2000/svg" | |
| > | |
| <path | |
| d="M0.900146 9.33203V12.0142C0.900146 12.3736 1.17392 12.6654 1.51126 12.6654H14.9557C15.1178 12.6654 15.2732 12.5968 15.3878 12.4746C15.5024 12.3525 15.5668 12.1869 15.5668 12.0142V10.3299L13.375 7.99523C13.1458 7.75134 12.8351 7.61436 12.5113 7.61436C12.1874 7.61436 11.8767 7.75134 11.6476 7.99523L10.1257 9.35919L10.2534 9.56204L11.7209 9.60056C11.7809 9.66017 11.8291 9.73206 11.8625 9.81194C11.8959 9.89181 11.9138 9.97804 11.9153 10.0655C11.9167 10.1529 11.9017 10.2397 11.8709 10.3208C11.8402 10.4019 11.7944 10.4756 11.7364 10.5374C11.6784 10.5992 11.6092 10.648 11.5332 10.6807C11.4571 10.7135 11.3756 10.7296 11.2935 10.728C11.2114 10.7265 11.1305 10.7073 11.0556 10.6717C10.9806 10.6362 10.9131 10.5848 10.8572 10.5209L10.2534 9.56204L6.60385 3.76614C6.37468 3.52226 6.11293 3.33203 5.78904 3.33203C5.46515 3.33203 5.20339 3.52226 4.97422 3.76614L0.900146 9.33203Z" | |
| fill="currentColor" | |
| /> | |
| <path | |
| d="M11.7209 9.60056L10.2534 9.56204L10.8572 10.5209C10.9131 10.5848 10.9806 10.6362 11.0556 10.6717C11.1305 10.7073 11.2114 10.7265 11.2935 10.728C11.3756 10.7296 11.4571 10.7135 11.5332 10.6807C11.6092 10.648 11.6784 10.5992 11.7364 10.5374C11.7944 10.4756 11.8402 10.4019 11.8709 10.3208C11.9017 10.2397 11.9167 10.1529 11.9153 10.0655C11.9138 9.97804 11.8959 9.89181 11.8625 9.81194C11.8291 9.73206 11.7809 9.66017 11.7209 9.60056Z" | |
| fill="currentColor" | |
| /> | |
| </svg> | |
| ); | |
| export const EpicIcon: React.FC<ISvgIcons> = ({ className = "text-current", ...rest }) => ( | |
| <svg | |
| className={className} | |
| viewBox="0 0 16 16" | |
| xmlns="http://www.w3.org/2000/svg" | |
| aria-hidden="true" | |
| role="img" | |
| {...rest} | |
| > | |
| <path | |
| d="M0.900146 9.33203V12.0142C0.900146 12.3736 1.17392 12.6654 1.51126 12.6654H14.9557C15.1178 12.6654 15.2732 12.5968 15.3878 12.4746C15.5024 12.3525 15.5668 12.1869 15.5668 12.0142V10.3299L13.375 7.99523C13.1458 7.75134 12.8351 7.61436 12.5113 7.61436C12.1874 7.61436 11.8767 7.75134 11.6476 7.99523L10.1257 9.35919L10.2534 9.56204L11.7209 9.60056C11.7809 9.66017 11.8291 9.73206 11.8625 9.81194C11.8959 9.89181 11.9138 9.97804 11.9153 10.0655C11.9167 10.1529 11.9017 10.2397 11.8709 10.3208C11.8402 10.4019 11.7944 10.4756 11.7364 10.5374C11.6784 10.5992 11.6092 10.648 11.5332 10.6807C11.4571 10.7135 11.3756 10.7296 11.2935 10.728C11.2114 10.7265 11.1305 10.7073 11.0556 10.6717C10.9806 10.6362 10.9131 10.5848 10.8572 10.5209L10.2534 9.56204L6.60385 3.76614C6.37468 3.52226 6.11293 3.33203 5.78904 3.33203C5.46515 3.33203 5.20339 3.52226 4.97422 3.76614L0.900146 9.33203Z" | |
| fill="currentColor" | |
| /> | |
| <path | |
| d="M11.7209 9.60056L10.2534 9.56204L10.8572 10.5209C10.9131 10.5848 10.9806 10.6362 11.0556 10.6717C11.1305 10.7073 11.2114 10.7265 11.2935 10.728C11.3756 10.7296 11.4571 10.7135 11.5332 10.6807C11.6092 10.648 11.6784 10.5992 11.7364 10.5374C11.7944 10.4756 11.8402 10.4019 11.8709 10.3208C11.9017 10.2397 11.9167 10.1529 11.9153 10.0655C11.9138 9.97804 11.8959 9.89181 11.8625 9.81194C11.8291 9.73206 11.7809 9.66017 11.7209 9.60056Z" | |
| fill="currentColor" | |
| /> | |
| </svg> | |
| ); |
Comment on lines
+3
to
+13
| export const getValueFromLocalStorage = (key: string, defaultValue: any) => { | ||
| if (typeof window === undefined || typeof window === "undefined") | ||
| return defaultValue; | ||
| try { | ||
| const item = window.localStorage.getItem(key); | ||
| return item ? JSON.parse(item) : defaultValue; | ||
| } catch (error) { | ||
| window.localStorage.removeItem(key); | ||
| return defaultValue; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Improve type safety and error handling
Several improvements can be made to this function:
- The typeof check is incorrect
- JSON.parse result needs type safety
- Error handling could be more informative
Apply these changes:
-export const getValueFromLocalStorage = (key: string, defaultValue: any) => {
- if (typeof window === undefined || typeof window === "undefined")
+export const getValueFromLocalStorage = <T>(key: string, defaultValue: T): T => {
+ if (typeof window === "undefined")
return defaultValue;
try {
const item = window.localStorage.getItem(key);
- return item ? JSON.parse(item) : defaultValue;
+ return item ? (JSON.parse(item) as T) : defaultValue;
} catch (error) {
+ console.warn(`Error reading ${key} from localStorage:`, error);
window.localStorage.removeItem(key);
return defaultValue;
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const getValueFromLocalStorage = (key: string, defaultValue: any) => { | |
| if (typeof window === undefined || typeof window === "undefined") | |
| return defaultValue; | |
| try { | |
| const item = window.localStorage.getItem(key); | |
| return item ? JSON.parse(item) : defaultValue; | |
| } catch (error) { | |
| window.localStorage.removeItem(key); | |
| return defaultValue; | |
| } | |
| }; | |
| export const getValueFromLocalStorage = <T>(key: string, defaultValue: T): T => { | |
| if (typeof window === "undefined") | |
| return defaultValue; | |
| try { | |
| const item = window.localStorage.getItem(key); | |
| return item ? (JSON.parse(item) as T) : defaultValue; | |
| } catch (error) { | |
| console.warn(`Error reading ${key} from localStorage:`, error); | |
| window.localStorage.removeItem(key); | |
| return defaultValue; | |
| } | |
| }; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: Invalid typeof comparison value: this expression is not a string literal
not a string literal
Unsafe fix: Compare the result of typeof with a valid type name
(lint/suspicious/useValidTypeof)
Comment on lines
+15
to
+24
| export const setValueIntoLocalStorage = (key: string, value: any) => { | ||
| if (typeof window === undefined || typeof window === "undefined") | ||
| return false; | ||
| try { | ||
| window.localStorage.setItem(key, JSON.stringify(value)); | ||
| return true; | ||
| } catch (error) { | ||
| return false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add type safety and improve error handling
Similar improvements needed as the get function:
- Fix typeof check
- Add type safety
- Improve error handling
Apply these changes:
-export const setValueIntoLocalStorage = (key: string, value: any) => {
- if (typeof window === undefined || typeof window === "undefined")
+export const setValueIntoLocalStorage = <T>(key: string, value: T): boolean => {
+ if (typeof window === "undefined")
return false;
try {
window.localStorage.setItem(key, JSON.stringify(value));
return true;
} catch (error) {
+ console.warn(`Error writing ${key} to localStorage:`, error);
return false;
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const setValueIntoLocalStorage = (key: string, value: any) => { | |
| if (typeof window === undefined || typeof window === "undefined") | |
| return false; | |
| try { | |
| window.localStorage.setItem(key, JSON.stringify(value)); | |
| return true; | |
| } catch (error) { | |
| return false; | |
| } | |
| }; | |
| export const setValueIntoLocalStorage = <T>(key: string, value: T): boolean => { | |
| if (typeof window === "undefined") | |
| return false; | |
| try { | |
| window.localStorage.setItem(key, JSON.stringify(value)); | |
| return true; | |
| } catch (error) { | |
| console.warn(`Error writing ${key} to localStorage:`, error); | |
| return false; | |
| } | |
| }; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 16-16: Invalid typeof comparison value: this expression is not a string literal
not a string literal
Unsafe fix: Compare the result of typeof with a valid type name
(lint/suspicious/useValidTypeof)
Comment on lines
+26
to
+59
| export const useLocalStorage = <T,>(key: string, initialValue: T) => { | ||
| const [storedValue, setStoredValue] = useState<T | null>(() => | ||
| getValueFromLocalStorage(key, initialValue) | ||
| ); | ||
|
|
||
| const setValue = useCallback( | ||
| (value: T) => { | ||
| window.localStorage.setItem(key, JSON.stringify(value)); | ||
| setStoredValue(value); | ||
| window.dispatchEvent(new Event(`local-storage:${key}`)); | ||
| }, | ||
| [key] | ||
| ); | ||
|
|
||
| const clearValue = useCallback(() => { | ||
| window.localStorage.removeItem(key); | ||
| setStoredValue(null); | ||
| window.dispatchEvent(new Event(`local-storage:${key}`)); | ||
| }, [key]); | ||
|
|
||
| const reHydrate = useCallback(() => { | ||
| const data = getValueFromLocalStorage(key, initialValue); | ||
| setStoredValue(data); | ||
| }, [key, initialValue]); | ||
|
|
||
| useEffect(() => { | ||
| window.addEventListener(`local-storage:${key}`, reHydrate); | ||
| return () => { | ||
| window.removeEventListener(`local-storage:${key}`, reHydrate); | ||
| }; | ||
| }, [key, reHydrate]); | ||
|
|
||
| return { storedValue, setValue, clearValue } as const; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and type safety in the hook
Several improvements needed in the hook implementation:
- Add error handling for localStorage operations
- Add type safety for custom events
- Ensure proper cleanup of event listeners
Apply these changes:
export const useLocalStorage = <T,>(key: string, initialValue: T) => {
const [storedValue, setStoredValue] = useState<T | null>(() =>
getValueFromLocalStorage(key, initialValue)
);
const setValue = useCallback(
(value: T) => {
+ try {
window.localStorage.setItem(key, JSON.stringify(value));
setStoredValue(value);
window.dispatchEvent(new Event(`local-storage:${key}`));
+ } catch (error) {
+ console.warn(`Error in setValue for ${key}:`, error);
+ }
},
[key]
);
const clearValue = useCallback(() => {
+ try {
window.localStorage.removeItem(key);
setStoredValue(null);
window.dispatchEvent(new Event(`local-storage:${key}`));
+ } catch (error) {
+ console.warn(`Error in clearValue for ${key}:`, error);
+ }
}, [key]);
const reHydrate = useCallback(() => {
const data = getValueFromLocalStorage(key, initialValue);
setStoredValue(data);
}, [key, initialValue]);
useEffect(() => {
+ const eventName = `local-storage:${key}`;
+ const handler = () => reHydrate();
+
- window.addEventListener(`local-storage:${key}`, reHydrate);
+ window.addEventListener(eventName, handler);
return () => {
- window.removeEventListener(`local-storage:${key}`, reHydrate);
+ window.removeEventListener(eventName, handler);
};
}, [key, reHydrate]);
return { storedValue, setValue, clearValue } as const;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const useLocalStorage = <T,>(key: string, initialValue: T) => { | |
| const [storedValue, setStoredValue] = useState<T | null>(() => | |
| getValueFromLocalStorage(key, initialValue) | |
| ); | |
| const setValue = useCallback( | |
| (value: T) => { | |
| window.localStorage.setItem(key, JSON.stringify(value)); | |
| setStoredValue(value); | |
| window.dispatchEvent(new Event(`local-storage:${key}`)); | |
| }, | |
| [key] | |
| ); | |
| const clearValue = useCallback(() => { | |
| window.localStorage.removeItem(key); | |
| setStoredValue(null); | |
| window.dispatchEvent(new Event(`local-storage:${key}`)); | |
| }, [key]); | |
| const reHydrate = useCallback(() => { | |
| const data = getValueFromLocalStorage(key, initialValue); | |
| setStoredValue(data); | |
| }, [key, initialValue]); | |
| useEffect(() => { | |
| window.addEventListener(`local-storage:${key}`, reHydrate); | |
| return () => { | |
| window.removeEventListener(`local-storage:${key}`, reHydrate); | |
| }; | |
| }, [key, reHydrate]); | |
| return { storedValue, setValue, clearValue } as const; | |
| }; | |
| export const useLocalStorage = <T,>(key: string, initialValue: T) => { | |
| const [storedValue, setStoredValue] = useState<T | null>(() => | |
| getValueFromLocalStorage(key, initialValue) | |
| ); | |
| const setValue = useCallback( | |
| (value: T) => { | |
| try { | |
| window.localStorage.setItem(key, JSON.stringify(value)); | |
| setStoredValue(value); | |
| window.dispatchEvent(new Event(`local-storage:${key}`)); | |
| } catch (error) { | |
| console.warn(`Error in setValue for ${key}:`, error); | |
| } | |
| }, | |
| [key] | |
| ); | |
| const clearValue = useCallback(() => { | |
| try { | |
| window.localStorage.removeItem(key); | |
| setStoredValue(null); | |
| window.dispatchEvent(new Event(`local-storage:${key}`)); | |
| } catch (error) { | |
| console.warn(`Error in clearValue for ${key}:`, error); | |
| } | |
| }, [key]); | |
| const reHydrate = useCallback(() => { | |
| const data = getValueFromLocalStorage(key, initialValue); | |
| setStoredValue(data); | |
| }, [key, initialValue]); | |
| useEffect(() => { | |
| const eventName = `local-storage:${key}`; | |
| const handler = () => reHydrate(); | |
| window.addEventListener(eventName, handler); | |
| return () => { | |
| window.removeEventListener(eventName, handler); | |
| }; | |
| }, [key, reHydrate]); | |
| return { storedValue, setValue, clearValue } as const; | |
| }; |
Comment on lines
+28
to
+44
| export const Tabs: FC<TTabsProps> = (props: TTabsProps) => { | ||
| const { | ||
| tabs, | ||
| storageKey, | ||
| actions, | ||
| defaultTab = tabs[0]?.key, | ||
| containerClassName = "", | ||
| tabListContainerClassName = "", | ||
| tabListClassName = "", | ||
| tabClassName = "", | ||
| tabPanelClassName = "", | ||
| } = props; | ||
| // local storage | ||
| const { storedValue, setValue } = useLocalStorage(`tab-${storageKey}`, defaultTab); | ||
|
|
||
| const currentTabIndex = (tabKey: string): number => tabs.findIndex((tab) => tab.key === tabKey); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and validation
The component needs better error handling:
- Add error boundary
- Handle empty tabs array
- Validate defaultTab value
Consider these improvements:
+import { ErrorBoundary } from "react-error-boundary";
export const Tabs: FC<TTabsProps> = (props: TTabsProps) => {
const {
tabs,
storageKey,
actions,
- defaultTab = tabs[0]?.key,
+ defaultTab,
containerClassName = "",
tabListContainerClassName = "",
tabListClassName = "",
tabClassName = "",
tabPanelClassName = "",
} = props;
+ if (!tabs.length) {
+ return null;
+ }
+ const effectiveDefaultTab = defaultTab || tabs[0].key;
+
// local storage
- const { storedValue, setValue } = useLocalStorage(`tab-${storageKey}`, defaultTab);
+ const { storedValue, setValue } = useLocalStorage(`tab-${storageKey}`, effectiveDefaultTab);
const currentTabIndex = (tabKey: string): number => tabs.findIndex((tab) => tab.key === tabKey);Committable suggestion skipped: line range outside the PR's diff.
rahulramesha
approved these changes
Dec 2, 2024
Merged
6 tasks
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changes:
This PR includes the following updates:
Reference:
[WEB-2799]
Summary by CodeRabbit
Release Notes
New Features
Tabscomponent for a tabbed interface with local storage support.CommentFillIcon,EpicIcon, andInfoFillIcon.LinearProgressIndicatorwith new size option and styling props.useLocalStoragehook for managing local storage.Improvements
CollapsibleButtonnow supports additional styling throughclassNameandtitleClassName.Documentation