refactor(web): clean up utils #1254
Conversation
WalkthroughThis pull request introduces several modifications across multiple components, primarily focusing on updating import paths, refining component structures, and enhancing styling. Notable changes include the removal of unused imports, the restructuring of components to eliminate unnecessary wrappers like Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
✅ Deploy Preview for reearth-web canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
web/src/beta/utils/string.ts (1)
Line range hint
6-14: Improve random string generation to avoid modulo biasThe current implementation of
generateRandomStringcould have slight bias in character distribution due to the modulo operation. While this might not be critical for general use, it's good practice to implement unbiased random string generation.Here's a more uniform implementation:
const usableChars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; const generateRandomString = (len: number): string => { - return Array.from(window.crypto.getRandomValues(new Uint8Array(len))) - .map((n) => usableChars[n % len]) + const mask = 63; // 2^6 - 1, as we have 62 chars + const values = new Uint8Array(len * 2); // Get extra bytes to account for rejected values + let result = ''; + let i = 0; + + window.crypto.getRandomValues(values); + + while (result.length < len) { + const rand = values[i] & mask; // Use bitmask instead of modulo + if (rand < usableChars.length) { + result += usableChars[rand]; + } + i++; + if (i >= values.length && result.length < len) { + // If we run out of random bytes, get more + window.crypto.getRandomValues(values); + i = 0; + } + } + return result.toLowerCase(); };This implementation:
- Uses a bitmask to avoid modulo bias
- Efficiently handles the case when random values need to be rejected
- Maintains cryptographic security
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/useAlias.ts (2)
Line range hint
7-65: Consider moving alias generation to the backendBased on previous learnings from PR #1238, the alias generation should be handled by the backend instead of the frontend. The current implementation:
- Generates random strings on the frontend
- Validates them against the backend
- Retries up to 3 times if collisions occur
This approach could lead to:
- Race conditions if multiple users generate aliases simultaneously
- Unnecessary network requests for validation
- Potential security issues if the alias format requirements change
Consider refactoring to use a backend endpoint that generates and validates aliases in a single request. This would:
- Reduce network traffic
- Eliminate race conditions
- Centralize alias generation logic
- Improve reliability by removing the retry mechanism
Would you like me to help draft a proposal for this architectural change?
Line range hint
67-69: Inline thegenerateAliasutility functionThe
generateAliasfunction is a simple wrapper that's only used once and adds unnecessary indirection.Consider inlining it where it's used:
-function generateAlias() { - return generateRandomString(10); -} // In createAlias: - const newAlias = generateAlias(); + const newAlias = generateRandomString(10);Also, consider documenting why 10 is chosen as the length, or make it configurable if it's a business requirement.
web/src/beta/features/Editor/index.tsx (2)
75-97: Consider memoizing EditorVisualizer to prevent unnecessary re-rendersThe EditorVisualizer component receives multiple props and callbacks which could trigger re-renders of this large component tree.
+ import { memo } from "react"; - const EditorVisualizer: FC<Props> = ({ ... }) => { + const EditorVisualizer: FC<Props> = memo(({ ... }) => { // component implementation - }; + });
100-119: Optimize bundle size with code splittingEach tab's content could be lazy loaded to improve initial page load performance, especially since only one tab is rendered at a time.
Example implementation:
import { lazy } from 'react'; const Map = lazy(() => import('./Map')); const Story = lazy(() => import('./Story')); const Widgets = lazy(() => import('./Widgets')); const Publish = lazy(() => import('./Publish'));Wrap the tab content with Suspense:
+ import { Suspense } from 'react'; {tab === "map" && ( <MapPageProvider value={mapPageValue}> + <Suspense fallback={<LoadingSpinner />}> <Map /> + </Suspense> </MapPageProvider> )}web/src/beta/features/AccountSetting/PasswordModal/index.tsx (3)
Line range hint
11-17: Consider enhancing password validationThe current password validation implementation could benefit from:
- Centralizing validation logic into a custom hook
- Adding default password policy patterns
- Documenting password requirements for users
Consider refactoring to:
// usePasswordValidation.ts export const defaultPasswordPolicy: PasswordPolicy = { tooShort: /^.{0,7}$/, tooLong: /^.{51,}$/, whitespace: /\s/, lowSecurity: /^[a-zA-Z0-9]*$/, medSecurity: /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d).*$/, highSecurity: /^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[@$!%*?&])[A-Za-z\d@$!%*?&]*$/ }; export const usePasswordValidation = (password: string, policy = defaultPasswordPolicy) => { // ... validation logic here };Also applies to: 42-65
Line range hint
116-122: Enhance user feedback for password requirementsConsider adding upfront visibility of password requirements to improve user experience:
- Show minimum requirements before user starts typing
- Use a checklist pattern to show progress towards a valid password
Consider adding a requirements checklist component:
const PasswordRequirements: React.FC<{ password: string, policy: PasswordPolicy }> = ({ password, policy }) => { return ( <RequirementsList> <RequirementItem met={!policy.tooShort?.test(password)}> {t("Minimum 8 characters")} </RequirementItem> {/* Add other requirements */} </RequirementsList> ); };Also applies to: 147-165
Line range hint
39-41: Address security considerationsConsider implementing these security enhancements:
- Add current password verification before allowing changes
- Clear password state on component unmount
- Add rate limiting for password change attempts
Example implementation:
useEffect(() => { return () => { // Clear sensitive data on unmount setPassword(""); setPasswordConfirmation(""); }; }, []); const handleSave = useCallback(async () => { if (attempts > MAX_ATTEMPTS) { showRateLimitError(); return; } if (await verifyCurrentPassword(currentPassword)) { handleUpdateUserPassword?.({ password, passwordConfirmation }); handleClose(); } }, [/* ... */]);Also applies to: 76-84
web/src/beta/features/Visualizer/Crust/Widgets/Widget/builtin/Button/MenuButton.tsx (1)
248-255: Consider enhancing type safety for menuType checks.The styled component implementation is clean and efficient. However, we could improve type safety for the menuType checks.
Consider using a type guard or switch statement to handle all possible menuType values explicitly:
- minHeight: item?.menuType === "border" ? undefined : "25px", + minHeight: (() => { + switch (item?.menuType) { + case "border": + return undefined; + case "link": + case "camera": + return "25px"; + default: + return "25px"; + } + })(),This approach ensures that all possible menuType values are handled explicitly and TypeScript can verify the exhaustiveness of the check.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (30)
web/src/beta/features/AccountSetting/PasswordModal/index.tsx(1 hunks)web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.tsx(1 hunks)web/src/beta/features/Editor/Map/DataSourceLayerCreator/util.ts(1 hunks)web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/useAlias.ts(1 hunks)web/src/beta/features/Editor/Story/PagesPanel/PageItem.tsx(1 hunks)web/src/beta/features/Editor/index.tsx(1 hunks)web/src/beta/features/Visualizer/Crust/StoryPanel/Block/builtin/Title/index.tsx(1 hunks)web/src/beta/features/Visualizer/Crust/Widgets/Widget/builtin/Button/MenuButton.tsx(2 hunks)web/src/beta/features/Visualizer/shared/components/BlockWrapper/hooks.ts(1 hunks)web/src/beta/hooks/useManageSwitchState/hooks.test.tsx(0 hunks)web/src/beta/hooks/useManageSwitchState/hooks.tsx(0 hunks)web/src/beta/pages/PublishedPage/index.tsx(1 hunks)web/src/beta/ui/widgetui/Flex/index.stories.tsx(0 hunks)web/src/beta/ui/widgetui/Flex/index.tsx(0 hunks)web/src/beta/utils/aria.ts(0 hunks)web/src/beta/utils/deepFind.test.ts(0 hunks)web/src/beta/utils/deepFind.ts(0 hunks)web/src/beta/utils/localstorage.ts(0 hunks)web/src/beta/utils/metrics.ts(0 hunks)web/src/beta/utils/raf.test.ts(0 hunks)web/src/beta/utils/raf.ts(0 hunks)web/src/beta/utils/string.ts(1 hunks)web/src/beta/utils/use-buffered.ts(0 hunks)web/src/beta/utils/use-dnd/drag.ts(0 hunks)web/src/beta/utils/use-dnd/drop.ts(0 hunks)web/src/beta/utils/use-dnd/index.tsx(0 hunks)web/src/beta/utils/use-dnd/types.ts(0 hunks)web/src/beta/utils/util.test.ts(0 hunks)web/src/beta/utils/util.ts(0 hunks)web/src/publishedapp.tsx(1 hunks)
💤 Files with no reviewable changes (18)
- web/src/beta/hooks/useManageSwitchState/hooks.test.tsx
- web/src/beta/hooks/useManageSwitchState/hooks.tsx
- web/src/beta/ui/widgetui/Flex/index.stories.tsx
- web/src/beta/ui/widgetui/Flex/index.tsx
- web/src/beta/utils/aria.ts
- web/src/beta/utils/deepFind.test.ts
- web/src/beta/utils/deepFind.ts
- web/src/beta/utils/localstorage.ts
- web/src/beta/utils/metrics.ts
- web/src/beta/utils/raf.test.ts
- web/src/beta/utils/raf.ts
- web/src/beta/utils/use-buffered.ts
- web/src/beta/utils/use-dnd/drag.ts
- web/src/beta/utils/use-dnd/drop.ts
- web/src/beta/utils/use-dnd/index.tsx
- web/src/beta/utils/use-dnd/types.ts
- web/src/beta/utils/util.test.ts
- web/src/beta/utils/util.ts
✅ Files skipped from review due to trivial changes (4)
- web/src/beta/features/Editor/Map/DataSourceLayerCreator/util.ts
- web/src/beta/features/Editor/Story/PagesPanel/PageItem.tsx
- web/src/beta/features/Visualizer/Crust/StoryPanel/Block/builtin/Title/index.tsx
- web/src/beta/features/Visualizer/shared/components/BlockWrapper/hooks.ts
🧰 Additional context used
📓 Learnings (1)
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/useAlias.ts (1)
Learnt from: airslice
PR: reearth/reearth-visualizer#1238
File: web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/useAlias.ts:18-29
Timestamp: 2024-11-12T15:21:04.151Z
Learning: When generating aliases for publishing (e.g., in `useAlias.ts`), the application will request the alias from the backend when necessary, instead of generating it on the frontend.
🪛 GitHub Check: ci-web / ci
web/src/beta/features/Dashboard/ContentsContainer/Projects/Project/hooks.tsx
[warning] 3-3:
@reearth/beta/hooks/useDoubleClick import should occur before import of @reearth/beta/lib/reearth-ui
🔇 Additional comments (11)
web/src/beta/pages/PublishedPage/index.tsx (2)
1-11: LGTM! Clean and minimal implementation.
The component has been simplified while maintaining its core functionality. The TypeScript interface is well-defined with optional parameters.
8-8: Verify the removal of DndProvider wrapper.
The DndProvider wrapper has been removed. Let's verify that drag-and-drop functionality is either handled at a higher level or is no longer needed.
✅ Verification successful
Let me try a different search strategy to verify the drag-and-drop functionality.
Let me try one more search with modified file type flags:
Let me try one final search without file type flags:
DndProvider removal is safe - component has been completely refactored
The git history shows that the DndProvider was removed along with PublishedGis component, and the entire component has been refactored to use Published component which renders a Visualizer. The new implementation doesn't use any drag-and-drop functionality, focusing instead on visualization features with Cesium engine.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if DndProvider is used elsewhere in the codebase and if Published component handles drag-and-drop internally
# Check for DndProvider usage in the codebase
echo "Checking for DndProvider usage:"
rg "DndProvider" -A 3
# Check if Published component handles drag-and-drop internally
echo -e "\nChecking Published component implementation:"
ast-grep --pattern 'import { $_ } from "react-dnd"' 'web/src/beta/features/Published'
Length of output: 285
Script:
#!/bin/bash
# Let's check the Published component implementation and its dependencies
# Check the Published component implementation
echo "Checking Published component implementation:"
fd "Published" web/src/beta/features --exec cat {}
# Look for any drag-and-drop related imports or usage
echo -e "\nChecking for react-dnd related imports:"
rg "react-dnd" --type ts --type tsx
# Check for drag-related props or handlers
echo -e "\nChecking for drag-related functionality:"
rg -i "drag|drop" --type ts --type tsx web/src/beta/features/Published
Length of output: 3693
Script:
#!/bin/bash
# Search for DnD and drag-drop related code with correct file type flags
# Check for react-dnd imports in the entire codebase
echo "Checking for react-dnd imports:"
rg "react-dnd" -t typescript -t typescriptreact
# Check for DndProvider usage
echo -e "\nChecking for DndProvider usage:"
rg "DndProvider" -t typescript -t typescriptreact
# Check for drag-drop related code in Published component and its parent
echo -e "\nChecking for drag-drop functionality in Published component:"
rg -i "drag|drop" -t typescript -t typescriptreact "web/src/beta/features/Published"
rg -i "drag|drop" -t typescript -t typescriptreact "web/src/beta/pages/PublishedPage"
Length of output: 697
Script:
#!/bin/bash
# Look for any DndProvider or react-dnd usage in the component or its parent
echo "Checking Published component and parent for DnD:"
rg -l "DndProvider|react-dnd" web/src/beta/features/Published web/src/beta/pages/PublishedPage
# Get the git history of the file to see what was removed
echo -e "\nChecking git history for DndProvider removal:"
git log -p web/src/beta/pages/PublishedPage/index.tsx | grep -A 5 "DndProvider"
# Check the Published component implementation
echo -e "\nChecking Published component source:"
cat web/src/beta/features/Published/index.tsx
Length of output: 2191
web/src/publishedapp.tsx (2)
9-11: LGTM! Component hierarchy has been simplified.
The removal of the unnecessary DndProvider wrapper results in a cleaner and more maintainable component structure.
9-11: Verify the removal of drag-and-drop functionality.
The removal of DndProvider suggests that drag-and-drop features are no longer needed in the published app. Let's verify this assumption.
✅ Verification successful
The removal of DndProvider appears safe in the published app
While drag-and-drop dependencies (react-dnd, react-beautiful-dnd) exist in package.json, there's no evidence of DnD usage in the PublishedPage component. The only drag-related code found is in the Timeline widget under the beta features, which uses simple mouse event handlers (onDrag) rather than the React DnD system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining drag-and-drop related code or dependencies
# Check for any drag-and-drop related imports or usage
echo "Checking for drag-and-drop related code..."
rg -i "drag|drop|dnd" --type tsx --type ts
# Check for any react-dnd dependencies in package.json
echo "Checking package.json for react-dnd dependencies..."
fd package.json --exec jq '.dependencies | with_entries(select(.key | test(".*dnd.*")))'
Length of output: 473
Script:
#!/bin/bash
# Let's check for actual usage of drag-and-drop in the codebase
echo "Checking for DnD imports and usage..."
rg -l "import.*react-(beautiful-)?dnd"
# Check specifically for DnD usage in PublishedPage component
echo "Checking PublishedPage component..."
fd "PublishedPage\.(tsx|ts)" --exec cat {}
# Check for any drag-drop event handlers or DnD hooks
echo "Checking for drag-drop event handlers and hooks..."
rg "use(Drag|Drop)"
rg "onDrag|onDrop"
Length of output: 1840
web/src/beta/utils/string.ts (1)
1-3: Verify the impact of the stricter empty string check
Since this function is being used in multiple components (PagesPanel/PageItem.tsx and StoryPanel/Block/Title/index.tsx), we should verify if the stricter whitespace handling might affect existing behavior.
✅ Verification successful
The stricter empty string check is safe to implement
The verification shows that isEmptyString is used consistently in both components to check for empty titles:
- In StoryPanel/Block/Title/index.tsx: Used to determine if a title has empty space via
hasEmptySpace - In PagesPanel/PageItem.tsx: Similarly used to detect empty titles via
hasEmptySpace
In both cases, the function is used to validate user input for titles, where treating whitespace-only strings as empty is the expected behavior. The stricter check aligns with the intended usage in these components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of isEmptyString to verify impact
# Look for patterns that might be affected by the stricter whitespace check
# Find direct usage of isEmptyString
echo "=== Direct usage of isEmptyString ==="
rg -l "isEmptyString"
# Look for test files that might need updating
echo -e "\n=== Related test files ==="
fd -e test.ts -e test.tsx | rg -l "string|empty"
# Look for strings with tabs or newlines being passed
echo -e "\n=== Potential problematic usage with special whitespace ==="
rg "isEmptyString\([^)]*[\t\n]"
Length of output: 563
Script:
#!/bin/bash
# Let's examine the actual usage of isEmptyString in the identified files
echo "=== Usage in StoryPanel/Block/Title/index.tsx ==="
rg -A 5 "isEmptyString" web/src/beta/features/Visualizer/Crust/StoryPanel/Block/builtin/Title/index.tsx
echo -e "\n=== Usage in PagesPanel/PageItem.tsx ==="
rg -A 5 "isEmptyString" web/src/beta/features/Editor/Story/PagesPanel/PageItem.tsx
# Let's also check if there are any existing tests for this utility
echo -e "\n=== Tests for string utils ==="
fd "string.test"
Length of output: 1539
web/src/beta/features/Editor/Publish/PublishToolsPanel/PublishOrUpdateModal/useAlias.ts (1)
1-1: Verify the new import path exists
The import path has been changed from generate-random-string to string. Let's verify this change is consistent across the codebase.
✅ Verification successful
Import path change is valid and consistent
The import path change from generate-random-string to string is correct and properly implemented:
- The new module
web/src/beta/utils/string.tsexists and exports thegenerateRandomStringfunction with identical functionality - No references to the old import path remain in the codebase
- The function is used consistently in two locations with the new import path
- The implementation in the new location maintains the same cryptographically secure random string generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new import path exists and check for any remaining old imports
# Check if the new module exists
fd -e ts -e tsx "string.ts" web/src/beta/utils/
# Check for any remaining old imports that need updating
rg -l "@reearth/beta/utils/generate-random-string"
Length of output: 127
Script:
#!/bin/bash
# Let's verify the content of both files and their usage
# Check the content of the new string.ts file
cat web/src/beta/utils/string.ts
# Search for any other imports of the new path to verify its usage
rg "@reearth/beta/utils/string" -A 2
# Search for generateRandomString function usage across the codebase
ast-grep --pattern 'generateRandomString($$$)'
Length of output: 2311
web/src/beta/features/Editor/index.tsx (3)
Line range hint 147-183: Styled components implementation looks good
The styled components make good use of theme variables and implement a robust layout structure using flexbox and absolute positioning.
Line range hint 22-28: Verify Props interface completeness
The Props interface seems minimal compared to the number of props used in the hooks. Consider documenting why certain props are extracted from hooks rather than passed directly.
#!/bin/bash
# Search for prop usages in the hooks file
echo "Checking hook implementation..."
rg "useHooks.*Props" "web/src/beta/features/Editor/hooks.ts"
# Search for EditorVisualizer prop types
echo "Checking EditorVisualizer props..."
ast-grep --pattern 'type Props = {
$$$
}'67-146: Verify drag-and-drop functionality after DndProvider removal
The removal of DndProvider might affect features like story block movement (handleStoryBlockMove) and other drag-and-drop interactions.
Consider implementing a more lightweight drag-and-drop solution or documenting why the DndProvider was removed to help future maintainers understand the architectural decision.
web/src/beta/features/AccountSetting/PasswordModal/index.tsx (1)
202-206: LGTM: Improved theme consistency
The styled component updates improve theme consistency by:
- Using theme tokens for spacing
- Adding proper gap spacing for icon alignment
web/src/beta/features/Visualizer/Crust/Widgets/Widget/builtin/Button/MenuButton.tsx (1)
179-188: LGTM! Clean and efficient menu item rendering.
The implementation follows React best practices with proper conditional rendering and spacing logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
web/.storybook/preview.tsx (1)
58-58: LGTM! Consider updating documentationThe removal of
DndProvideraligns with the PR's cleanup objectives and simplifies the Storybook setup.Consider adding a comment in the file explaining why drag-and-drop functionality was removed from the global decorators, to help future maintainers understand this architectural decision.
(Story) => { return ( <ApolloProvider client={mockClient}> <I18nProvider> + {/* DndProvider removed as part of utils cleanup - drag-and-drop should be handled at individual story level if needed */} <Story /> </I18nProvider> </ApolloProvider> ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
web/.storybook/preview.tsx(1 hunks)
🔇 Additional comments (1)
web/.storybook/preview.tsx (1)
58-58: Verify stories that use drag-and-drop functionality
The removal of DndProvider might affect stories that rely on drag-and-drop features.
Let's check for potentially affected stories:
✅ Verification successful
Removal of DndProvider is safe for existing stories
The Timeline story uses onDrag as a simple callback prop for time updates, not for HTML5 drag-and-drop functionality. It doesn't require the React DnD's DndProvider. The story works independently of the DnD context, as it's just using the prop for time manipulation.
- The only story file with drag-related terms is
Timeline/UI/index.stories.tsx - It uses
onDragas a time update callback, not for DnD features - No
DndProviderimports or DnD-specific code found in the story
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find stories that might use drag-and-drop functionality
# Look for keywords related to drag and drop in story files
echo "Searching for drag-and-drop related stories..."
rg -l --glob "**/*.stories.{tsx,jsx}" -e "drag|drop|dnd|draggable|droppable|onDrag|onDrop"
Length of output: 287
Script:
#!/bin/bash
# Let's examine the timeline story file to understand its drag-and-drop usage
echo "Checking Timeline story content:"
cat "web/src/beta/features/Visualizer/Crust/Widgets/Widget/builtin/Timeline/UI/index.stories.tsx"
# Also search for DndProvider imports in this file
echo -e "\nChecking DndProvider imports:"
rg "DndProvider" "web/src/beta/features/Visualizer/Crust/Widgets/Widget/builtin/Timeline/UI/index.stories.tsx"
Length of output: 1407
Overview
There're some not-in-use utils and hooks codes, this PR cleans up some of them.
What I've done
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
isEmptyStringto check for empty or whitespace-only strings.Bug Fixes
PasswordModalcomponent for improved dynamic styling based on the theme.MenuButtoncomponent for better alignment and clarity.Refactor
DndProviderin various components, includingPublishedPageandApp.Chores