fix(web): time disappear on cancel timeline popup #1220
Conversation
WalkthroughThe changes in this pull request primarily focus on the 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 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
🧹 Outside diff range and nitpick comments (4)
web/src/beta/ui/fields/TimePeriodField/EditModal.tsx (2)
63-63: Consider adding type safety to onClose handlerWhile the direct use of
onCloseis correct, consider adding a type check to handle potential undefined callbacks gracefully.-<Button onClick={onClose} size="normal" title="Cancel" /> +<Button onClick={() => onClose?.()} size="normal" title="Cancel" />
Line range hint
36-63: Consider adding test coverage for cancellation flowThe simplified cancellation flow looks good, but it would be beneficial to add test cases that verify:
- Time period values are preserved when canceling the modal
- The modal closes correctly with both the cancel button and panel actions
- Edge cases like multiple rapid cancellations
This will help prevent regression of the fixed issue.
web/src/beta/ui/fields/TimePeriodField/index.tsx (2)
33-38: Consider strengthening the null check condition.The condition
if (!value)might be too broad as it evaluates to true for bothundefinedandnull. Consider being more explicit about the check to prevent unintended state resets.- if (!value) { + if (value === undefined) { setTimePeriodValues(undefined); }
Line range hint
109-117: Consider optimizing modal rendering and prop passing.The current implementation passes the state setter directly to the modal and uses a redundant visibility prop. Consider these improvements:
- Memoize the modal component to prevent unnecessary rerenders
- Remove the redundant
visibleprop since the modal is already conditionally rendered- {openEditModal && ( - <EditModal - timePeriodValues={timePeriodValues} - onChange={onChange} - onClose={handleEditorModalClose} - visible={openEditModal} - setTimePeriodValues={setTimePeriodValues} - /> - )} + {openEditModal && ( + <EditModal + timePeriodValues={timePeriodValues} + onChange={onChange} + onClose={handleEditorModalClose} + setTimePeriodValues={setTimePeriodValues} + /> + )}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
web/src/beta/ui/fields/TimePeriodField/EditModal.tsx(2 hunks)web/src/beta/ui/fields/TimePeriodField/hooks.ts(0 hunks)web/src/beta/ui/fields/TimePeriodField/index.tsx(1 hunks)web/src/services/theme/reearthTheme/common/index.ts(2 hunks)
💤 Files with no reviewable changes (1)
- web/src/beta/ui/fields/TimePeriodField/hooks.ts
✅ Files skipped from review due to trivial changes (1)
- web/src/services/theme/reearthTheme/common/index.ts
🔇 Additional comments (4)
web/src/beta/ui/fields/TimePeriodField/EditModal.tsx (2)
Line range hint 36-41: LGTM: Simplified hook usage
The removal of handleClose from hooks destructuring helps streamline the modal's cancellation flow, reducing potential state management issues.
60-60: LGTM: Direct modal cancellation handling
Using onClose directly in onCancel ensures proper state management during modal cancellation.
Let's verify the modal cancellation behavior across the codebase:
✅ Verification successful
LGTM: Consistent modal cancellation pattern confirmed
The codebase search reveals that the direct use of close/cancel handlers in onCancel prop is a common pattern across the codebase. Examples include:
onCancel={onClose}in AssetsSelector, ProjectDeleteModal, SketchLayerCreatoronCancel={handleClose}in PasswordModal, PluginInstall, SketchLayerEditor- Direct arrow functions that set state or handle cleanup
The change in TimePeriodField's EditModal aligns with these established patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of ModalPanel usage to ensure consistent cancellation handling
ast-grep --pattern 'ModalPanel {
$$$
onCancel={$_}
$$$
}'
Length of output: 65
Script:
#!/bin/bash
# Let's try a different approach to find ModalPanel usage patterns
# First, let's find files containing ModalPanel
rg -l "ModalPanel"
# Then search for onCancel prop usage with surrounding context
rg "onCancel=\{" -A 2 -B 2
Length of output: 10564
web/src/beta/ui/fields/TimePeriodField/index.tsx (2)
32-38: Consider implementing error handling and validation.
The component could benefit from:
- Input validation for time period values
- Error boundaries for graceful failure handling
- Loading states during async operations
This would improve the robustness and user experience of the component.
Let's check if there are any existing error boundaries or validation patterns in the codebase:
#!/bin/bash
# Search for error boundary implementations
rg -l "class.*ErrorBoundary"
# Search for time validation patterns
rg "time.*validation" Also applies to: 45-48
33-38: Verify the fix addresses the original bug completely.
While the implementation looks correct, please verify that:
- The time period values persist when canceling with existing data
- The state synchronization doesn't cause unexpected updates
Let's check for similar issues in the codebase:
Also applies to: 45-48
✅ Verification successful
Fix correctly handles time period persistence and state synchronization
After analyzing the component's implementation, the fix is verified to be complete and correct because:
- The
handleEditorModalCloseonly resets time period values when there's no externalvalueprop - State synchronization is properly maintained through the
useEffecthook that updates internal state when the externalvaluechanges - The component correctly preserves existing time period data when canceling with data present
- The delete functionality is properly separated and only triggers when explicitly called via
handleTimePeriodSettingDelete
The implementation ensures time values won't disappear unexpectedly on modal close while maintaining proper state synchronization with the parent component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar state reset patterns
ast-grep --pattern 'setTimePeriodValues(undefined)'
# Search for related bug reports
rg -i "time.*disappear.*cancel"
Length of output: 265
Script:
#!/bin/bash
# Let's analyze the component's state management and modal interactions
ast-grep --pattern 'const TimePeriodField = ({ $$ }) => {
$$$
}'
# Check for related state updates in modal interactions
rg -l "setOpenEditModal.*false" --type tsx
# Look for similar modal close handlers
ast-grep --pattern 'const handle$_ModalClose = $_(() => {
$$$
})'
Length of output: 211
Script:
#!/bin/bash
# Let's examine the component's implementation more thoroughly
cat web/src/beta/ui/fields/TimePeriodField/index.tsx
# Search for similar patterns with correct file type
rg -l "setOpenEditModal.*false" -t ts -t tsx -t jsx
# Look for related test files
fd "TimePeriodField.test" -e tsx -e ts
Length of output: 5012
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
web/src/beta/ui/fields/TimePeriodField/hooks.ts (1)
Line range hint
89-97: Enhance validation in handleSubmit.The submit validation only checks for non-empty strings but doesn't ensure the times form a valid range (start ≤ current ≤ end). This could allow invalid time periods to be saved.
Consider adding range validation:
const handleSubmit = useCallback(() => { if ( timePeriodValues?.currentTime !== "" && timePeriodValues?.startTime !== "" && timePeriodValues?.endTime !== "" && + new Date(timePeriodValues.startTime) <= new Date(timePeriodValues.currentTime) && + new Date(timePeriodValues.currentTime) <= new Date(timePeriodValues.endTime) ) { onChange?.(timePeriodValues); onClose?.(); } }, [timePeriodValues, onChange, onClose]);web/src/beta/ui/fields/TimePeriodField/index.tsx (1)
Line range hint
9-19: Consider architectural improvements for better maintainability.The component mixes UI rendering with state management concerns. Consider these improvements:
- Add runtime type validation for the time format in TimePeriodFieldProp
- Consider using a custom hook to manage the time period logic
- Split the progress steps visualization into a separate component
Example type validation:
const isValidTimeFormat = (time: string): boolean => { // Add your time format validation logic here return /^\d{2}:\d{2}(:\d{2})?$/.test(time); }; export type TimePeriodFieldProp = { currentTime: string; startTime: string; endTime: string; validate?: () => boolean; // Add validation method };Also applies to: 46-50
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
web/src/beta/ui/fields/TimePeriodField/EditModal.tsx(2 hunks)web/src/beta/ui/fields/TimePeriodField/hooks.ts(1 hunks)web/src/beta/ui/fields/TimePeriodField/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/beta/ui/fields/TimePeriodField/EditModal.tsx
🔇 Additional comments (3)
web/src/beta/ui/fields/TimePeriodField/hooks.ts (1)
60-62: Verify cancel behavior in parent component.
The removal of local state management means the parent component must properly handle the cancel case to fix the reported bug where "time disappears on cancel timeline popup".
Let's verify the parent component's implementation:
✅ Verification successful
Cancel behavior is properly handled in the component
The code shows that cancel behavior is properly implemented:
-
In
TimePeriodField/index.tsx,handleEditorModalClosecorrectly handles the cancel case:const handleEditorModalClose = useCallback(() => { setOpenEditModal(false); if (!value) { onChange?.(undefined); } }, [onChange, value]);
-
The EditModal component in
EditModal.tsxproperly passes the cancel action through itsonCloseprop to the Modal component, which triggershandleEditorModalClose. -
The component maintains local state (
timePeriodValues) that's synced with the parent's value through useEffect, ensuring the original value is preserved on cancel:useEffect(() => { setTimePeriodValues(value); }, [value]);
The reported bug "time disappears on cancel timeline popup" should not occur with this implementation as the component properly preserves the original state on cancel.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the parent component handles the cancel case
# Expected: Find implementations of onClose and onChange in parent components
# Search for TimePeriodField usage
rg -A 10 "TimePeriodField" --type ts --type tsx
# Search for specific cancel handling
rg -A 5 "handleEditorModalClose|handleTimePeriodSettingDelete" --type ts --type tsx
Length of output: 189
Script:
#!/bin/bash
# Let's try without file type filters since tsx is not recognized
# Search for TimePeriodField usage and surrounding context
rg -A 10 "TimePeriodField"
# Also search for files containing TimePeriodField in their name
fd "TimePeriodField"
Length of output: 9690
Script:
#!/bin/bash
# Let's check the implementation of TimePeriodField component to see how it handles cancel
cat web/src/beta/ui/fields/TimePeriodField/index.tsx
# Also check the EditModal implementation
cat web/src/beta/ui/fields/TimePeriodField/EditModal.tsx
Length of output: 9016
web/src/beta/ui/fields/TimePeriodField/index.tsx (2)
41-44: LGTM! Clean and efficient implementation.
The delete handler implementation is well-structured with:
- A proper guard clause to prevent unnecessary updates
- Consistent state management through the onChange callback
33-38: 🛠️ Refactor suggestion
Review the modal close logic to prevent unintended state resets.
The current implementation might lead to unintended state resets. The condition if (!value) will trigger onChange(undefined) whenever the modal is closed and no value prop exists, which might not always be the desired behavior.
Consider this alternative implementation that better preserves the state:
const handleEditorModalClose = useCallback(() => {
setOpenEditModal(false);
- if (!value) {
- onChange?.(undefined);
- }
}, [onChange, value]);Let's verify if this change affects other parts of the codebase:
…re/timeline-block
…ualizer into chore/timeline-block
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
web/src/beta/ui/fields/TimePeriodField/EditModal.tsx(4 hunks)web/src/beta/ui/fields/TimePeriodField/hooks.ts(3 hunks)web/src/beta/ui/fields/TimePeriodField/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/beta/ui/fields/TimePeriodField/EditModal.tsx
- web/src/beta/ui/fields/TimePeriodField/index.tsx
🔇 Additional comments (3)
web/src/beta/ui/fields/TimePeriodField/hooks.ts (3)
19-22: Previous null safety concern still applies.
94-98: LGTM! Null safety properly handled.
The isDisabled implementation correctly handles the null case and properly validates all fields.
104-108: LGTM! Return object properly updated.
The returned object correctly reflects the new state management approach with localValue exposed and handleClose removed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
web/src/beta/ui/fields/TimePeriodField/hooks.ts (2)
Line range hint
23-64: Consider using a more robust time comparison approach.While the null safety improvement is good, the current time comparison using substring manipulation could be fragile. Consider using a dedicated date-time library or the Date object's methods directly.
Example approach:
- new Date(updatedData.currentTime.substring(0, 19)) < new Date(updatedData.startTime.substring(0, 19)) + const currentDate = new Date(updatedData.currentTime); + const startDate = new Date(updatedData.startTime); + currentDate.getTime() < startDate.getTime()
98-102: Consider a more concise implementation of isDisabled.While the current implementation is correct, it could be more concise.
- const isDisabled = useMemo(() => { - if (localValue) { - return Object.values(localValue).every((value) => value !== ""); - } - return false; - }, [localValue]); + const isDisabled = useMemo(() => + localValue ? Object.values(localValue).every(Boolean) : false + , [localValue]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
web/src/beta/ui/fields/TimePeriodField/hooks.ts(3 hunks)
🔇 Additional comments (2)
web/src/beta/ui/fields/TimePeriodField/hooks.ts (2)
11-19: LGTM! Props interface simplification improves component maintainability.
The removal of setTimePeriodValues and addition of local state management with proper prop synchronization is a good architectural decision that helps prevent the time disappearance issue.
108-112: LGTM! Return value changes align with new state management.
The addition of localValue and removal of handleClose from returned values is consistent with the new state management approach.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (17)
web/src/beta/lib/reearth-ui/components/ClickAway/index.tsx (2)
29-29: Enhance component flexibility and accessibility.Consider these improvements:
- Allow customization of the wrapper element type
- Add ARIA attributes for accessibility
- Support style forwarding
Here's the suggested implementation:
+type WrapperProps = { + as?: keyof JSX.IntrinsicElements; + className?: string; + style?: React.CSSProperties; +}; + -type ClickAwayProps = { +type ClickAwayProps = WrapperProps & { children: ReactNode; onClickAway?: () => void; }; -export const ClickAway: FC<ClickAwayProps> = ({ children, onClickAway }) => { +export const ClickAway: FC<ClickAwayProps> = ({ + as: Component = 'div', + children, + className, + style, + onClickAway +}) => { // ... rest of the implementation ... - return <div ref={containerRef}>{children}</div>; + return ( + <Component + ref={containerRef} + className={className} + style={style} + role="presentation" + > + {children} + </Component> + ); };
1-30: Implementation aligns well with PR objectives.The
ClickAwaycomponent provides a robust solution for handling outside clicks in the timeline popup, which should effectively fix the issue of time data disappearing on cancel. The implementation is clean, reusable, and follows React best practices.Consider documenting usage examples in the component's directory to help other developers understand how to properly integrate it with modals and popups.
web/src/beta/lib/reearth-ui/components/index.ts (1)
Line range hint
22-22: Remove duplicate export of PopupMenu component.The
PopupMenucomponent is exported twice in this file (lines 22 and 26).export * from "./PopupMenu"; export * from "./DragAndDropList"; export * from "./Tabs"; export * from "./Radio"; export * from "./RadioGroup"; -export * from "./PopupMenu";Also applies to: 26-26
web/src/beta/lib/reearth-ui/components/PopupPanel/index.tsx (1)
23-24: Consider defensive handling of onCancel prop.To make the component more robust, consider adding a guard for undefined
onCancel.- <ClickAway onClickAway={onCancel}> + <ClickAway onClickAway={onCancel ?? (() => {})}>web/src/beta/ui/fields/TimePointField/EditPanel/hooks.ts (2)
8-12: Add JSDoc comments to document the Props interface.Consider adding JSDoc comments to document the purpose and expected format of each prop, especially the
valueparameter which should follow a specific datetime format.+/** + * Props for the TimePointField EditPanel hook + * @param value - Optional datetime string in format "YYYY-MM-DDThh:mm±hhmm" + * @param onChange - Optional callback when datetime value changes + * @param onClose - Callback to handle panel closure + */ type Props = { value?: string; onChange?: (value?: string | undefined) => void; onClose: () => void; };
14-19: Add input validation for the value prop.The hook should validate the incoming datetime string format to ensure it matches the expected pattern "YYYY-MM-DDThh:mm±hhmm". This would prevent potential runtime errors from malformed input.
export default ({ value, onChange, onClose }: Props) => { + const isValidDateTimeFormat = (val: string) => { + return /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}[+-]\d{4}$/.test(val); + }; + const [date, setDate] = useState(""); const [time, setTime] = useState(""); const [timezone, setTimezone] = useState<TimeZoneOffset>( getLocalTimezoneOffset() );web/src/beta/lib/reearth-ui/components/TimePicker/index.tsx (1)
52-53: Input constraints look good, consider some improvements.The time constraints are valid and provide good input validation. Consider these enhancements:
- Extract the time constraints as constants
- Add error handling for invalid inputs
- Add aria-labels for accessibility
+const TIME_CONSTRAINTS = { + MIN: "00:00:00", + MAX: "23:59:59", +} as const; <StyledInput value={currentValue} disabled={disabled} onChange={handleChange} onBlur={handleBlur} onFocus={handleFocus} type="time" - min="00:00:00" - max="23:59:59" + min={TIME_CONSTRAINTS.MIN} + max={TIME_CONSTRAINTS.MAX} + aria-label="Time input" step={1} />web/src/beta/ui/fields/TimePointField/index.tsx (1)
Line range hint
56-90: UI changes look good, consider adding aria-label.The UI changes properly implement the controlled component pattern and fix the cancel behavior. However, the buttons could benefit from accessibility improvements.
Consider adding aria-labels to improve accessibility:
<Button key="delete" icon="trash" size="small" iconButton appearance="simple" + aria-label={t("delete time")} disabled={!localValue} onClick={handleTimeSettingDelete} iconColor={localValue ? theme.content.main : theme.content.weak} />web/src/beta/utils/time.ts (2)
46-77: Consider generating timezone offsets programmatically.While the current implementation is correct, maintaining a hard-coded list of time zones can be error-prone. Consider generating the offsets programmatically for better maintainability.
-export const TIMEZONE_OFFSETS = [ - "-12:00", - "-11:00", - // ... more entries - "+14:00" -] as const; +export const TIMEZONE_OFFSETS = Array.from({ length: 27 }, (_, i) => { + const offset = i - 12; + const sign = offset >= 0 ? "+" : "-"; + const hours = String(Math.abs(offset)).padStart(2, "0"); + return `${sign}${hours}:00`; +}) as const;
90-92: Consider more flexible timezone validation.The current implementation only accepts exact matches from TIMEZONE_OFFSETS. Consider adding support for:
- Normalizing input (trimming whitespace, standardizing format)
- Parsing and validating the offset format itself
export const isValidTimezone = (timezone: string): boolean => { - return TIMEZONE_OFFSETS.includes(timezone as TimeZoneOffset); + const normalized = timezone.trim(); + if (TIMEZONE_OFFSETS.includes(normalized as TimeZoneOffset)) return true; + + // Validate format: (+/-)(HH):(MM) + const regex = /^[-+]\d{2}:\d{2}$/; + if (!regex.test(normalized)) return false; + + const [hours, minutes] = normalized.substring(1).split(':').map(Number); + return hours >= 0 && hours <= 14 && minutes >= 0 && minutes < 60; };web/src/beta/ui/fields/TimePointField/EditPanel/index.tsx (1)
70-74: Consider adding timezone display enhancementWhile the timezone selector implementation is correct, consider enhancing it to show both the timezone offset and a friendly name for better user understanding.
options={TIMEZONE_OFFSETS.map((offset) => ({ - label: offset, + label: `${offset} (${getTimezoneDisplayName(offset)})`, value: offset }))}web/src/beta/ui/fields/TimePeriodField/EditPanel/index.tsx (2)
57-72: Consider adding time format validationWhile the local state management looks good, consider adding validation to ensure consistent time format across all fields.
Consider adding a time format validator:
const isValidTimeFormat = (time: string): boolean => { // Add time format validation logic return true; };
Line range hint
74-82: Consider enhancing warning message accessibilityThe warning message implementation is good, but consider adding aria-labels for better accessibility.
- <Warning> + <Warning role="alert" aria-live="polite">web/src/beta/ui/fields/TimePeriodField/EditPanel/hooks.ts (2)
12-20: Consider adding cleanup on unmount.While the state management is good, consider resetting the state when the component unmounts to prevent any potential memory leaks or stale state issues.
useEffect(() => { setLocalValue(timePeriodValues); + return () => { + setLocalValue(undefined); + setTimeRangeInvalid(false); + }; }, [timePeriodValues]);
79-97: Refactor submitDisabled for better readability.While the logic is correct, the function could be reorganized for better maintainability.
const submitDisabled = useMemo(() => { + const hasRequiredFields = + localValue?.startTime && + localValue?.currentTime && + localValue?.endTime; + + if (!hasRequiredFields || timeRangeInvalid) { + return true; + } + + const timezones = new Set([ + getTimeZone(localValue.startTime), + getTimeZone(localValue.currentTime), + getTimeZone(localValue.endTime) + ]); + + return timezones.size !== 1; - if ( - !localValue?.startTime || - !localValue?.currentTime || - !localValue?.endTime || - timeRangeInvalid - ) { - return true; - } - - const startTimezone = getTimeZone(localValue?.startTime); - const currentTimezone = getTimeZone(localValue?.currentTime); - const endTimezone = getTimeZone(localValue?.endTime); - if (startTimezone !== currentTimezone || currentTimezone !== endTimezone) { - return true; - } - - return false; }, [localValue, timeRangeInvalid]);web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts (2)
Line range hint
147-162: Ensure timezone persistence on cancel.The useEffect implementation now properly handles timezone updates when timelineValues change. However, to prevent the reported bug where time disappears on cancel:
- Consider memoizing the timezone value to prevent unnecessary updates
- Add a check to prevent timezone updates when canceling
useEffect(() => { if (timelineValues) { const iso8601Time = formatISO8601(timelineValues?.currentTime); const t = getNewDate(new Date(iso8601Time)).getTime(); const timezoneOffset = getTimeZone(iso8601Time); - setTimezone(timezoneOffset); + // Only update timezone if it actually changed + setTimezone(prev => prev !== timezoneOffset ? timezoneOffset : prev); return setCurrentTime(t); } else { return setCurrentTime( getNewDate( visualizerContext?.current?.timeline?.current?.timeline?.current ).getTime() ); } }, [timelineValues, visualizerContext]);
Line range hint
164-180: Consider performance optimizations.The hook manages multiple pieces of state and callbacks. To optimize performance and prevent unnecessary re-renders:
- Memoize the return object using useMemo
- Consider using useCallback for setCurrentTime
- return { + return useMemo(() => ({ currentTime, range, playSpeedOptions, speed, timezone, onPlay, onSpeedChange: handleOnSpeedChange, onPause, onTimeChange, onTick, removeTickEventListener, onCommit, removeOnCommitEventListener, - setCurrentTime - }; + setCurrentTime: useCallback((time: number) => setCurrentTime(time), []) + }), [ + currentTime, range, playSpeedOptions, speed, timezone, + onPlay, handleOnSpeedChange, onPause, onTimeChange, + onTick, removeTickEventListener, onCommit, removeOnCommitEventListener + ]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
web/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (16)
web/package.json(0 hunks)web/src/beta/features/Visualizer/Crust/StoryPanel/utils.ts(1 hunks)web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts(2 hunks)web/src/beta/lib/reearth-ui/components/ClickAway/index.tsx(1 hunks)web/src/beta/lib/reearth-ui/components/PopupPanel/index.tsx(2 hunks)web/src/beta/lib/reearth-ui/components/TimePicker/index.tsx(1 hunks)web/src/beta/lib/reearth-ui/components/index.ts(1 hunks)web/src/beta/ui/fields/TimePeriodField/EditPanel/hooks.ts(1 hunks)web/src/beta/ui/fields/TimePeriodField/EditPanel/index.tsx(2 hunks)web/src/beta/ui/fields/TimePeriodField/index.tsx(3 hunks)web/src/beta/ui/fields/TimePointField/EditPanel/hooks.ts(1 hunks)web/src/beta/ui/fields/TimePointField/EditPanel/index.tsx(4 hunks)web/src/beta/ui/fields/TimePointField/hooks.ts(0 hunks)web/src/beta/ui/fields/TimePointField/index.tsx(3 hunks)web/src/beta/utils/moment-timezone.ts(0 hunks)web/src/beta/utils/time.ts(1 hunks)
💤 Files with no reviewable changes (3)
- web/package.json
- web/src/beta/ui/fields/TimePointField/hooks.ts
- web/src/beta/utils/moment-timezone.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/beta/ui/fields/TimePeriodField/index.tsx
🔇 Additional comments (22)
web/src/beta/lib/reearth-ui/components/ClickAway/index.tsx (1)
3-6: LGTM! Clean and well-typed props interface.
The type definition is clear, concise, and follows TypeScript best practices.
web/src/beta/lib/reearth-ui/components/index.ts (1)
31-31: LGTM! The ClickAway component export enhances popup interaction handling.
The addition of the ClickAway component export aligns well with the PR's objective of fixing the time data persistence issue in popups by providing better control over popup dismissal events.
web/src/beta/lib/reearth-ui/components/PopupPanel/index.tsx (2)
1-1: LGTM! Clean import organization.
The addition of ClickAway import is well-organized by combining it with the existing Button import from the same module.
23-38: Verify handling of undefined onCancel prop.
The implementation looks good and aligns with the PR objective. However, since onCancel is optional in PopupPanelProps, we should verify its handling when undefined.
✅ Verification successful
Based on the search results, I can generate the final response as I have sufficient information. All instances of PopupPanel usage in the codebase consistently provide the onCancel prop, so there's no risk of undefined handler issues.
No issues with undefined onCancel prop
All current usages of PopupPanel properly provide the onCancel prop, making the implementation safe. The component is being used correctly throughout the codebase in:
- Camera position editor and capture panels
- Color picker
- Time point editor
- Action panel settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for PopupPanel usage without onCancel prop
rg -l "PopupPanel" | xargs rg "<PopupPanel" -A 5
Length of output: 3751
web/src/beta/ui/fields/TimePointField/EditPanel/hooks.ts (2)
61-76: LGTM! Well-structured return value.
The return object is well-organized, providing a clean interface for the component to access state and handlers. The applyDisabled computation is properly memoized and includes all necessary validation checks.
1-76: Verify the fix prevents time data loss on cancel.
While the implementation looks correct, let's verify that this hook properly maintains time data when the cancel action is triggered.
✅ Verification successful
Time data is properly preserved on cancel
The implementation correctly prevents time data loss on cancel:
- The
TimePointFieldcomponent maintains a local state (localValue) that only updates when the parent'svalueprop changes - The
EditPanelis isolated and only triggersonChangewhen explicitly applying changes - Cancellation flow is properly handled:
- Cancel button in
EditPanelonly callsonClose - Closing the panel doesn't modify the parent state
- No state updates occur during cancel operations
- Cancel button in
The code follows best practices for state management and user interactions, ensuring data persistence until explicitly changed by the user.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for cancel-related code to ensure proper state handling
echo "Searching for cancel handling in related components..."
rg -l "onClose" --type ts --type tsx src/beta/ui/fields/
rg "handleCancel|onCancel" --type ts --type tsx src/beta/ui/fields/
Length of output: 301
Script:
#!/bin/bash
# Search for cancel handling in related components with correct file extensions
echo "Searching for TimePointField components and cancel handling..."
fd . "web/src/beta/ui/fields" -e ts -e tsx -x rg -l "onClose|handleCancel|onCancel" {}
# Look for TimePointField component usage to understand cancel flow
echo -e "\nSearching for TimePointField usage..."
rg "TimePointField" -A 3 -B 3
# Check EditPanel implementation
echo -e "\nChecking EditPanel implementation..."
fd "EditPanel" "web/src/beta/ui/fields/TimePointField" -e ts -e tsx -x cat {}
Length of output: 9584
Script:
#!/bin/bash
# Let's check the TimePointField index and EditPanel implementation specifically
echo "Checking TimePointField implementation..."
cat web/src/beta/ui/fields/TimePointField/index.tsx
echo -e "\nChecking EditPanel index..."
cat web/src/beta/ui/fields/TimePointField/EditPanel/index.tsx
Length of output: 5667
web/src/beta/lib/reearth-ui/components/TimePicker/index.tsx (1)
52-53: Verify the relationship to the bug fix.
While these constraints improve input validation, they don't seem to directly address the PR's objective of fixing time disappearance when canceling. Please clarify how these changes contribute to the bug fix.
web/src/beta/ui/fields/TimePointField/index.tsx (2)
31-35: Good implementation of controlled component pattern.
The introduction of localValue with prop synchronization helps prevent unwanted data loss when canceling edits, directly addressing the PR's bug fix objective.
12-18: LGTM! Simplified prop interface improves component clarity.
The removal of complex field management props in favor of simpler disabled/enabled state and editor control is a good improvement. The new TimePointFieldRef type suggests better encapsulation of editor state.
Let's verify no breaking changes in parent components:
✅ Verification successful
These broader searches will help us find all usages of TimePointField and verify if the removed props were being used anywhere.
Props removal is safe - no breaking changes detected
Based on the search results, all current usages of TimePointField only use the maintained props (title, description, value, onChange) and none of the removed props (fieldName, onTimePointPopupOpen, setDisabledFields). The component is used in:
- useFieldComponent.tsx
- ProprtyField.tsx
- TimePeriodField/EditPanel/index.tsx (3 instances)
- fields/index.stories.tsx (storybook)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for removed props usage in parent components
rg "TimePointField.*fieldName|TimePointField.*onTimePointPopupOpen|TimePointField.*setDisabledFields"
Length of output: 103
Script:
#!/bin/bash
# Let's try a broader search to find TimePointField usage
rg "<TimePointField" -A 5 -B 5
Length of output: 5982
Script:
#!/bin/bash
# Also search for TimePointField imports to find all files using it
rg "import.*TimePointField"
Length of output: 139
Script:
#!/bin/bash
# Search for any references to TimePointField in the codebase
rg "TimePointField" --type typescript --type tsx
Length of output: 83
web/src/beta/ui/fields/TimePointField/EditPanel/index.tsx (3)
8-8: LGTM: Good move to use internal time utilities
The switch to using TIMEZONE_OFFSETS from internal utilities instead of moment-timezone is a good modernization step that helps maintain consistency across the codebase.
46-48: Good addition of disabled state control
The applyDisabled state for the Apply button improves user experience by preventing invalid submissions.
21-32: Verify that time values are preserved on cancel
The removal of setDateTime and simplified state management looks good, but let's verify that this change properly fixes the time preservation issue when canceling.
web/src/beta/ui/fields/TimePeriodField/EditPanel/index.tsx (4)
6-7: LGTM: Import and prop changes align with local state management
The removal of setTimePeriodValues and switch to relative imports improve the component's encapsulation.
41-51: LGTM: Improved modal action handling
The simplified cancel handling and proper disable state for the apply button prevent unintended state updates.
17-35: Verify the fix prevents time disappearance on cancel
The implementation looks correct, but let's verify there are no side effects from the state management changes.
#!/bin/bash
# Description: Check for similar patterns that might need the same fix
# Expected: Find similar modal/popup implementations with time-related state
# Look for similar time-related modal implementations
echo "Searching for similar time-related modals..."
rg -l "TimePeriod.*Modal" --type typescript
# Check for potential similar cancel handling patterns
echo "Checking similar cancel handling patterns..."
rg "onCancel.*time" --type typescript -A 526-34: Verify the hook's state management implementation
The introduction of localValue and validation states looks good, but let's verify the hook implementation to ensure proper state persistence.
✅ Verification successful
State management implementation verified and approved
The hook implementation shows proper state management with:
- Local state initialization using
timePeriodValues - Synchronization with parent through
useEffect - Proper validation in
handleChange - Clean submission logic that only updates parent on explicit submit
- Comprehensive validation before enabling submit button
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the hooks implementation to verify state management
# Expected: Find the hooks file and verify state initialization
# Look for the hooks file
echo "Searching for hooks implementation..."
rg -l "useHooks" --type typescript
# Check state initialization in hooks
echo "Checking state management in hooks..."
rg "const.*localValue" --type typescript -A 5
Length of output: 358
Script:
#!/bin/bash
# Let's try different approaches to find the hooks implementation
# Search for useHooks implementation in the same directory
fd "index.tsx|hooks.ts" web/src/beta/ui/fields/TimePeriodField/EditPanel/ --type f --exec cat {}
# Search for localValue state initialization with ripgrep
rg "localValue.*=.*useState" web/src/beta/ui/fields/TimePeriodField/EditPanel/ -A 5
# Look for state management patterns in the component directory
rg "useHooks" web/src/beta/ui/fields/TimePeriodField/EditPanel/ -A 10
Length of output: 8407
web/src/beta/ui/fields/TimePeriodField/EditPanel/hooks.ts (2)
1-10: LGTM! Clean type definitions and imports.
The Props interface is well-defined with appropriate optional properties, and imports are properly organized.
99-106: LGTM! Clean hook return structure.
The hook returns all necessary values and handlers in a clear, organized manner.
web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts (2)
Line range hint 1-180: Verify the bug fix with test cases.
To ensure the time no longer disappears when canceling the timeline popup, please verify these scenarios:
- Setting time periods and clicking cancel
- Changing timezone and clicking cancel
- Edge cases with different timezone offsets
#!/bin/bash
# Description: Look for related test cases and UI components
# Check for timeline popup component tests
echo "Checking timeline popup tests:"
rg -g "*.test.*" "timeline.*popup" "web/src/beta/features/"
# Check for related UI components
echo "Checking related components:"
rg "TimePeriodField" "web/src/beta/features/"1-1: Verify the new time utility implementation.
The getTimeZone function has been moved to a dedicated time utilities module, which is a good architectural decision. However, we should verify its implementation to ensure it handles all edge cases correctly.
Also applies to: 14-14
✅ Verification successful
Import change is correct and implementation is properly used
The getTimeZone function is correctly implemented in web/src/beta/utils/time.ts and is being used appropriately in useTimelineBlock.ts. The function extracts timezone offset from ISO8601 time strings using regex pattern matching, which is exactly what's needed for the timezone handling in the timeline popup bug fix. The implementation:
- Correctly extracts timezone offsets like "+00:00" or "-07:00" from ISO8601 strings
- Returns empty string as fallback when no timezone is found
- Is used properly in the hook's useEffect to update timezone state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of getTimeZone utility
# and its test coverage
# Check the implementation
echo "Checking getTimeZone implementation:"
rg -A 10 "export.*getTimeZone" "web/src/beta/utils/time.ts"
# Check for test coverage
echo "Checking test coverage:"
rg -g "*.test.*" "getTimeZone" "web/src/beta/utils/"
Length of output: 512
Script:
#!/bin/bash
# Let's try a different approach to locate the time utility implementation
# First, let's find all time-related utility files
echo "Searching for time utility files:"
fd "time\.ts" "web/src/beta"
# Then search for getTimeZone implementation using ast-grep
echo "Searching for getTimeZone function implementation:"
ast-grep --pattern 'export function getTimeZone() {
$$$
}'
# Also search for any getTimeZone references
echo "Searching for getTimeZone references:"
rg "getTimeZone" "web/src/beta"
# Check for any test files containing time in their name
echo "Searching for time-related test files:"
fd "time.*test\.(ts|tsx)$" "web/src/beta"
Length of output: 1553
Script:
#!/bin/bash
# Let's check the actual implementation of getTimeZone in time.ts
echo "Checking getTimeZone implementation:"
cat "web/src/beta/utils/time.ts"
# Let's also check the usage context in useTimelineBlock
echo -e "\nChecking useTimelineBlock implementation:"
cat "web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts"
Length of output: 8251
web/src/beta/features/Visualizer/Crust/StoryPanel/utils.ts (2)
2-2: LGTM! Good refactoring to use centralized time utility.
Moving the time zone logic to a shared utility follows DRY principles and improves maintainability.
Line range hint 164-177: Verify time zone handling consistency.
The function correctly handles time zone formatting, but given this PR's focus on fixing time-related issues, let's verify the behavior across different time zone formats.
Let's check the imported getTimeZone implementation and its usage:
✅ Verification successful
Time zone handling implementation is correct and consistent across the codebase
The implementation of getTimeZone in web/src/beta/utils/time.ts correctly extracts timezone offsets using a regex pattern ([-+]\d{1,2}:\d{2})$, and its usage is consistent across all files:
- In
StoryPanel/utils.ts: Used for standardizing timezone format in ISO8601 strings - In
useTimelineBlock.ts: Used for timeline synchronization - In
TimePeriodField/hooks.ts: Used for validating consistent timezones across time period ranges
The function properly handles both formats mentioned in the comment (with and without space before timezone) and standardizes 2-digit hour formats to ensure consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify time zone handling implementation and usage
# Check the implementation of the imported getTimeZone function
echo "Checking getTimeZone implementation:"
rg -A 10 "export.*getTimeZone.*=.*" "src/beta/utils/time.ts"
# Check all usages of getTimeZone to ensure consistent behavior
echo -e "\nChecking getTimeZone usage patterns:"
rg "getTimeZone\(" --type ts
Length of output: 904
Script:
#!/bin/bash
# Let's check the actual implementation of getTimeZone in the web directory
echo "Searching for getTimeZone implementation in web directory:"
rg -A 10 "export.*getTimeZone" web/
# Let's also check the actual usage context in the files where it's used
echo -e "\nChecking usage context in StoryPanel/utils.ts:"
rg -B 5 -A 5 "getTimeZone" "web/src/beta/features/Visualizer/Crust/StoryPanel/utils.ts"
echo -e "\nChecking usage context in useTimelineBlock.ts:"
rg -B 5 -A 5 "getTimeZone" "web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts"
echo -e "\nChecking usage context in TimePeriodField hooks:"
rg -B 5 -A 5 "getTimeZone" "web/src/beta/ui/fields/TimePeriodField/EditPanel/hooks.ts"
Length of output: 2810
…ualizer into chore/timeline-block
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
web/src/beta/ui/fields/TimePointField/EditPanel/hooks.ts (2)
22-30: Add error handling for invalid time/date inputs.The change handlers silently return on falsy values. Consider providing feedback to the user or logging these cases.
const handleTimeChange = useCallback((newValue: string) => { - if (!newValue) return; + if (!newValue) { + console.debug('Invalid time value received'); + return; + } setTime(newValue); }, []); const handleDateChange = useCallback((newValue: string) => { - if (!newValue) return; + if (!newValue) { + console.debug('Invalid date value received'); + return; + } setDate(newValue); }, []);
43-60: Extract timezone parsing logic to a utility function.The timezone parsing logic is complex and could be moved to a separate utility function for better maintainability and reusability.
+const parseTimezoneFromDateTime = (timeWithOffset: string): TimeZoneOffset => { + const [, timezoneOffset] = timeWithOffset.split(/[-+]/); + return (timeWithOffset.includes("-") + ? `-${timezoneOffset}` + : `+${timezoneOffset}`) as TimeZoneOffset; +}; useEffect(() => { if (value && isValidDateTimeFormat(value)) { const [parsedDate, timeWithOffset] = value.split("T"); const [parsedTime] = timeWithOffset.split(/[-+]/); setDate(parsedDate); setTime(parsedTime); - setTimezone( - (timeWithOffset.includes("-") - ? `-${timezoneOffset}` - : `+${timezoneOffset}`) as TimeZoneOffset - ); + setTimezone(parseTimezoneFromDateTime(timeWithOffset)); } else {web/src/beta/utils/time.ts (1)
94-101: Normalize timezone format for consistency.While the error handling is good, the function should normalize single-digit hours to ensure consistent format (e.g., "+09:00" instead of "+9:00").
export const getTimeZone = (time: string): TimeZoneOffset | undefined => { const zone = time.match(/([-+]\d{1,2}:\d{2})$/)?.[1]; + if (!zone) return undefined; + + // Normalize to standard format + const normalized = zone.replace( + /^([-+])(\d):/, + (_, sign, hour) => `${sign}0${hour}` + ); + return !zone ? undefined - : isValidTimezone(zone) - ? (zone as TimeZoneOffset) + : isValidTimezone(normalized) + ? (normalized as TimeZoneOffset) : undefined; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
web/src/beta/features/Visualizer/Crust/StoryPanel/utils.ts(2 hunks)web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts(3 hunks)web/src/beta/lib/reearth-ui/components/ClickAway/index.tsx(1 hunks)web/src/beta/ui/fields/TimePeriodField/EditPanel/hooks.ts(1 hunks)web/src/beta/ui/fields/TimePointField/EditPanel/hooks.ts(1 hunks)web/src/beta/utils/time.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/beta/features/Visualizer/Crust/StoryPanel/utils.ts
- web/src/beta/features/Visualizer/shared/hooks/useTimelineBlock.ts
- web/src/beta/lib/reearth-ui/components/ClickAway/index.tsx
- web/src/beta/ui/fields/TimePeriodField/EditPanel/hooks.ts
🔇 Additional comments (6)
web/src/beta/ui/fields/TimePointField/EditPanel/hooks.ts (4)
1-13: LGTM! Well-structured imports and type definitions.
The imports are properly organized, and the Props interface clearly defines the component's contract.
37-41: Add validation in handleApply.
The past review comment about validation in handleApply is still valid. The function should validate the formatted datetime before submission.
62-77: LGTM! Clean implementation of disabled state and return value.
The disabled state computation is comprehensive, and the return object is well-structured with all necessary values and handlers.
15-20: Verify state persistence on cancel.
The state management looks good, but given the PR's objective to fix time disappearing on cancel, let's verify that this implementation maintains state correctly.
✅ Verification successful
Let me gather more specific information about the TimePointField component and its state management.
Based on the code inspection, I can now generate the final response:
State persistence on cancel is properly implemented
The TimePointField component correctly maintains state through its EditPanel. The implementation shows:
- Initial state is properly set from the existing value when the panel opens
- State is only updated through explicit user actions
- The
onClosehandler is cleanly separated from state updates - Cancel operation simply closes the panel without modifying the state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for cancel-related state management in the codebase
rg -l "onClose" --type ts --type tsx | xargs rg -A 5 "cancel"
# Look for related components that might affect state persistence
ast-grep --pattern 'const $_ = ({ onClose }) => { $$$ }'
Length of output: 14250
Script:
#!/bin/bash
# Look for the TimePointField component implementation and related files
fd TimePointField | xargs rg -A 10 "onClose"
# Search for state management in TimePointField
ast-grep --pattern 'const [$_] = useState($$$)'
Length of output: 6044
web/src/beta/utils/time.ts (2)
79-88: Skip: Issue already identified.
90-92: LGTM! Type-safe timezone validation.
The implementation correctly leverages TypeScript's type system to ensure type safety when validating timezone offsets.
Overview
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
ClickAwaycomponent for enhanced interactivity, allowing actions on clicks outside specified areas.TIMEZONE_OFFSETSand various utility functions for improved time zone management.Improvements
TimePickerto restrict input values to valid time ranges.TimePeriodFieldandTimePointFieldcomponents.Bug Fixes
Chores
moment-timezonedependency to simplify project management.