refactor(web): improve exit story block edit mode #1347
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (2)
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 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1347 +/- ##
==========================================
- Coverage 23.00% 22.80% -0.21%
==========================================
Files 1041 1046 +5
Lines 107732 108908 +1176
Branches 663 668 +5
==========================================
+ Hits 24786 24836 +50
- Misses 81687 82812 +1125
- Partials 1259 1260 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
web/src/beta/features/Visualizer/Crust/StoryPanel/hooks.ts (2)
139-142: LGTM! Consider adding a comment for clarity.The selection reset logic is well-placed and handles the cleanup appropriately. Consider adding a brief comment explaining why we reset these states during page changes when selection is disabled.
+ // Reset selection states when changing pages with disabled selection if (disableSelection) { setDisableSelection(false); setSelectedBlockId(undefined); }
Line range hint
139-170: Great architectural consistency!The changes maintain a consistent pattern with other selection handlers in the codebase. The selection state management is well-coordinated across page changes, block selection, and double-click handlers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/beta/features/Visualizer/Crust/StoryPanel/hooks.ts(2 hunks)
🔇 Additional comments (1)
web/src/beta/features/Visualizer/Crust/StoryPanel/hooks.ts (1)
170-170: LGTM! Dependency array is correctly updated.The addition of
disableSelectionto the dependency array is necessary and properly placed.
airslice
changed the title
…re/story-blockexit-mode
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/src/beta/features/Visualizer/Crust/StoryPanel/PanelContent/hooks.ts (1)
Line range hint
16-83: Consider splitting the hook into smaller, focused hooks.The current hook handles multiple concerns:
- Page gap calculations
- Block creation/deletion
- Selection state
- Scroll timeout
Consider splitting these into separate hooks for better maintainability and testability:
usePageGapuseBlockOperationsuseSelectionStateExample refactor:
const usePageGap = () => { const [pageGap, setPageGap] = useState<number>(); // Page gap calculation logic return pageGap; }; const useBlockOperations = (onBlockCreate, onBlockDelete) => { // Block creation/deletion logic return { handleBlockCreate, handleBlockDelete }; }; const useSelectionState = () => { const editModeContext = useEditModeContext(); // Selection state logic return { disableSelection }; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/beta/features/Visualizer/Crust/StoryPanel/PanelContent/hooks.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
- GitHub Check: ci-web / ci
- GitHub Check: ci-server / ci-server-test
- GitHub Check: ci-server / ci-server-lint
🔇 Additional comments (1)
web/src/beta/features/Visualizer/Crust/StoryPanel/PanelContent/hooks.ts (1)
61-61:⚠️ Potential issueFix inconsistent pageGap calculations.
There's an inconsistency in the pageGap calculations:
- Initial render subtracts 90px
- Resize handler subtracts 40px
- Comments contradict each other about the page title block height
This will cause layout shifts when the window is resized.
Please verify the correct page title block height and ensure consistent calculations. Apply this fix once verified:
- if (pageWrapperElement) setPageGap(pageWrapperElement.clientHeight - 90); // 90px is the height of the page title block + if (pageWrapperElement) setPageGap(pageWrapperElement.clientHeight - TITLE_BLOCK_HEIGHT); // Height of the page title blockAnd in the resize handler:
- if (pageWrapperElement) setPageGap(pageWrapperElement.clientHeight - 40); // 40px is the height of the page title block + if (pageWrapperElement) setPageGap(pageWrapperElement.clientHeight - TITLE_BLOCK_HEIGHT); // Height of the page title blockConsider extracting the title block height to a constant to maintain consistency:
const TITLE_BLOCK_HEIGHT = 90; // or 40, depending on the correct valueLet's verify the correct height:
…re/story-blockexit-mode
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/src/beta/features/Visualizer/Crust/StoryPanel/constants.ts (1)
6-6: Consider grouping related constants together.The new constant breaks the logical grouping of constants. Consider moving it next to the other story page gap-related constants for better code organization.
Apply this diff to improve constant grouping:
export const STORY_PANEL_WIDTH = 442; export const STORY_PANEL_CONTENT_ELEMENT_ID = "story-page-content"; -export const DEFULT_STORY_PAGE_GAP = 320 // Story Page export const DEFAULT_STORY_PAGE_DURATION = 2; export const DEFAULT_STORY_PAGE_GAP = 20; +export const DEFAULT_STORY_PAGE_EDIT_GAP = 320; // Used for scroll behavior in edit mode
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/src/beta/features/Visualizer/Crust/StoryPanel/constants.ts(1 hunks)web/src/beta/features/Visualizer/Crust/StoryPanel/hooks.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/beta/features/Visualizer/Crust/StoryPanel/hooks.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
- GitHub Check: ci-server / ci-server-test
- GitHub Check: ci-server / ci-server-lint
- GitHub Check: ci-web / ci
🔇 Additional comments (1)
web/src/beta/features/Visualizer/Crust/StoryPanel/constants.ts (1)
6-6: Verify the gap value's impact on UI.The new gap value (320) is significantly larger than the default gap (20). Please ensure this larger value doesn't cause any UI issues, especially on smaller screens.
Run the following script to find any UI-related tests or responsive design checks:
✅ Verification successful
The gap value (320px) is intentionally large for smooth page transitions.
The gap is specifically used for scroll offset during page navigation and doesn't affect the layout or cause any UI issues. The story panel has a fixed width of 442px, and the scrolling behavior is well-managed with smooth transitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for UI tests and responsive design checks related to story panel gaps # Look for story panel related tests rg -l "StoryPanel.*test" --type ts # Look for responsive design related code rg "(?i)(responsive|mobile|screen|width|mediaquery)" --type ts "web/src/beta/features/Visualizer/Crust/StoryPanel"Length of output: 4492
Script:
#!/bin/bash # Find usages of DEFULT_STORY_PAGE_GAP and scroll/pagination related code # Find direct references to DEFULT_STORY_PAGE_GAP rg "DEFULT_STORY_PAGE_GAP" --type ts -A 5 -B 5 # Look for scroll and pagination related code in StoryPanel rg "(?i)(scroll|pagination|gap|margin|space)" --type ts "web/src/beta/features/Visualizer/Crust/StoryPanel" -A 3 -B 3Length of output: 54210
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
web/src/beta/features/Visualizer/Crust/StoryPanel/PanelContent/hooks.ts (1)
Fix inconsistent page title block height calculations
There's a bug where the page title block height is calculated differently:
- Initial load: Uses 90px
- Window resize: Uses 40px
This inconsistency will cause layout jumps when the window is resized, as confirmed by the inline comments stating both measurements are for the "page title block".
🔗 Analysis chain
Line range hint
61-73: Verify the impact of pageGap changes on the story panel layout.The change from 40px to 90px in the pageGap calculation represents a significant layout adjustment. Please ensure this doesn't negatively impact the story panel's appearance or functionality.
Let's check for any related layout calculations or styles:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related layout calculations and styles echo "Searching for related pageGap usage..." rg -t typescript -t tsx "pageGap" "web/src/beta/features/Visualizer" echo "Searching for related style definitions..." rg -t typescript -t tsx "90px|40px" "web/src/beta/features/Visualizer/Crust/StoryPanel"Length of output: 406
Script:
#!/bin/bash # Search with correct file extensions echo "Searching for pageGap usage..." rg "pageGap" --type-add 'ts:*.{ts,tsx}' --type=ts "web/src/beta/features/Visualizer" echo -e "\nSearching for related style definitions..." rg "90px|40px" --type-add 'ts:*.{ts,tsx}' --type=ts --type-add 'css:*.{css,scss}' --type=css "web/src/beta/features/Visualizer/Crust/StoryPanel" echo -e "\nSearching for styled-components definitions..." rg "styled\." --type-add 'ts:*.{ts,tsx}' --type=ts "web/src/beta/features/Visualizer/Crust/StoryPanel"Length of output: 1555
Script:
#!/bin/bash # Search for PageGap component definition echo "Searching for PageGap component..." rg "PageGap" --type-add 'ts:*.{ts,tsx}' --type=ts "web/src/beta/features/Visualizer" echo -e "\nSearching for comments about measurements..." rg -B 2 -A 2 "(40|90)px" --type-add 'ts:*.{ts,tsx}' --type=ts "web/src/beta/features/Visualizer"Length of output: 2958
🧹 Nitpick comments (2)
web/src/beta/features/Visualizer/Crust/StoryPanel/PanelContent/hooks.ts (2)
Line range hint
61-73: Consider extracting the pageGap calculation logic.The pageGap calculation is duplicated between useLayoutEffect and the resize handler. This could lead to maintenance issues if the calculation needs to change in the future.
Consider extracting the calculation into a shared function:
const calculatePageGap = (element: HTMLElement | null) => { const PAGE_TITLE_HEIGHT = 90; // px return element ? element.clientHeight - PAGE_TITLE_HEIGHT : undefined; };Then use it in both places:
useLayoutEffect(() => { const pageWrapperElement = document.getElementById( STORY_PANEL_CONTENT_ELEMENT_ID ); - if (pageWrapperElement) setPageGap(pageWrapperElement.clientHeight - 90); + setPageGap(calculatePageGap(pageWrapperElement)); }, [setPageGap]); useEffect(() => { const resizeCallback = () => { const pageWrapperElement = document.getElementById( STORY_PANEL_CONTENT_ELEMENT_ID ); - if (pageWrapperElement) setPageGap(pageWrapperElement.clientHeight - 40); + setPageGap(calculatePageGap(pageWrapperElement)); }; window.addEventListener("resize", resizeCallback); return () => window.removeEventListener("resize", resizeCallback); }, []);
Line range hint
64-73: Consider debouncing the resize handler.The resize handler is called on every resize event, which could lead to performance issues due to frequent state updates.
Consider using a debounce utility to limit the frequency of updates:
import { debounce } from "lodash"; // In the hook: useEffect(() => { const resizeCallback = debounce(() => { const pageWrapperElement = document.getElementById( STORY_PANEL_CONTENT_ELEMENT_ID ); setPageGap(calculatePageGap(pageWrapperElement)); }, 100); // Adjust delay as needed window.addEventListener("resize", resizeCallback); return () => { resizeCallback.cancel(); // Clean up debounce timer window.removeEventListener("resize", resizeCallback); }; }, []);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/src/beta/features/Visualizer/Crust/StoryPanel/PanelContent/hooks.ts(1 hunks)web/src/beta/features/Visualizer/Crust/StoryPanel/hooks.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/beta/features/Visualizer/Crust/StoryPanel/hooks.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: ci-web / ci
- GitHub Check: ci-server / ci-server-test
- GitHub Check: ci-server / ci-server-lint
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
web/src/beta/features/Visualizer/Crust/StoryPanel/PanelContent/hooks.ts
Outdated
Show resolved
Hide resolved
…re/story-blockexit-mode
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/src/beta/features/Visualizer/Crust/StoryPanel/PanelContent/index.tsx (1)
155-156: Enhance the StoryPageWrapper styling for better responsiveness and user experience.The current styling is minimal and could benefit from additional improvements:
const StoryPageWrapper = styled("div")(() => ({ - height: "100vh" + height: "100vh", + height: "-webkit-fill-available", // Add fallback for mobile browsers + maxHeight: "100vh", // Prevent overflow on desktop + overflow: "hidden", // Prevent potential scrolling issues + transition: "opacity 0.3s ease", // Smooth transitions between pages + "@media (max-width: 768px)": { + height: "100dvh" // Use dynamic viewport height on modern browsers + } }));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/src/beta/features/Visualizer/Crust/StoryPanel/PanelContent/hooks.ts(0 hunks)web/src/beta/features/Visualizer/Crust/StoryPanel/PanelContent/index.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- web/src/beta/features/Visualizer/Crust/StoryPanel/PanelContent/hooks.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: ci-server / ci-server-lint
- GitHub Check: ci-server / ci-server-test
- GitHub Check: ci-web / ci
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
🔇 Additional comments (1)
web/src/beta/features/Visualizer/Crust/StoryPanel/PanelContent/index.tsx (1)
109-133: Verify alignment with PR objectives.The changes appear to focus on page layout and styling, while the PR objective mentions improving the exit story block edit mode. Could you clarify how these changes contribute to improving the exit story block edit mode? The only visible connection is the
disableSelectionprop usage.Let's verify the edit mode related changes:
Also applies to: 155-156
✅ Verification successful
Changes align with PR objectives
The changes are correctly implementing the improved exit story block edit mode through the
disableSelectionprop, which is part of a coordinated mechanism between edit mode and selection states. The code shows that:
- Selection is disabled when exiting edit mode
- Edit mode is exited when selection is disabled
- These props are properly propagated through the component hierarchy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for edit mode related code rg -g '*.{ts,tsx}' '(editMode|disableSelection|handleCurrentPageChange)' # Search for related tests rg -g '*test*.{ts,tsx}' '(editMode|disableSelection|handleCurrentPageChange)'Length of output: 14547
web/src/beta/features/Visualizer/Crust/StoryPanel/PanelContent/index.tsx
Show resolved
Hide resolved
…re/story-blockexit-mode
…th-visualizer into chore/story-blockexit-mode
Overview
What I've done
What I haven't done
How I tested
Screen.Recording.2025-01-13.at.19.27.36.mov
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Bug Fixes
Refactor