add timeline view #4965
add timeline view #4965
Conversation
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- app/packages/core/src/plugins/SchemaIO/components/PlotlyView.tsx (0 hunks)
- app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx (1 hunks)
- app/packages/core/src/plugins/SchemaIO/components/index.ts (1 hunks)
- fiftyone/operators/types.py (1 hunks)
💤 Files with no reviewable changes (1)
- app/packages/core/src/plugins/SchemaIO/components/PlotlyView.tsx
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/plugins/SchemaIO/components/index.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (2)
app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx (1)
1-4: LGTM! Clean imports following best practices.app/packages/core/src/plugins/SchemaIO/components/index.ts (1)
52-52: LGTM! The export follows established patterns.The new TimelineView export maintains consistency with existing exports and follows proper alphabetical ordering.
Let's verify the TimelineView component exists:
✅ Verification successful
TimelineView component exists and follows React patterns
The verification confirms:
- TimelineView.tsx exists in the correct directory
- The component is properly implemented as a React functional component
- It follows the expected patterns with proper props handling and configuration management
The export in the barrel file is correctly referencing an existing and well-structured component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify TimelineView component exists and follows React patterns # Expected: Find the TimelineView.tsx file in the same directory fd -t f "TimelineView.tsx" "app/packages/core/src/plugins/SchemaIO/components" # Also verify the component implementation ast-grep --pattern 'export default function TimelineView($_) { $$$ }'Length of output: 2846
app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx
Outdated
Show resolved
Hide resolved
| const requiredParams = ["timeline_name", "total_frames"]; | ||
|
|
||
| // for (const param of requiredParams) { | ||
| // if (!finalConfig[param]) { | ||
| // throw new Error(`Missing required parameter: ${param}`); | ||
| // } | ||
| // } |
There was a problem hiding this comment.
Implement or remove commented validation code
The commented validation code seems important for ensuring required parameters are present. Either implement it properly or remove it if not needed. Consider using TypeScript's type system for validation instead.
- const requiredParams = ["timeline_name", "total_frames"];
-
- // for (const param of requiredParams) {
- // if (!finalConfig[param]) {
- // throw new Error(`Missing required parameter: ${param}`);
- // }
- // }
+ if (!timeline_name || !total_frames) {
+ throw new Error('Missing required parameters: timeline_name and total_frames are required');
+ }Committable suggestion was skipped due to low confidence.
| export const TimelineCreator = ({ timelineName, totalFrames, loop }) => { | ||
| const { isTimelineInitialized } = useCreateTimeline({ | ||
| name: timelineName, | ||
| config: { | ||
| totalFrames: totalFrames, | ||
| loop, | ||
| }, | ||
| }); | ||
|
|
||
| if (!isTimelineInitialized) { | ||
| return <div>initializing timeline...</div>; | ||
| } | ||
|
|
||
| return <Timeline name={timelineName} />; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance TimelineCreator component robustness and performance
Consider the following improvements:
- Add error handling for timeline initialization failures
- Improve loading state feedback
- Optimize performance with useMemo
- export const TimelineCreator = ({ timelineName, totalFrames, loop }) => {
+ export const TimelineCreator = React.memo(({ timelineName, totalFrames, loop }) => {
- const { isTimelineInitialized } = useCreateTimeline({
+ const { isTimelineInitialized, error } = useCreateTimeline({
name: timelineName,
config: {
totalFrames: totalFrames,
loop,
},
});
+ if (error) {
+ return <div>Error initializing timeline: {error.message}</div>;
+ }
if (!isTimelineInitialized) {
- return <div>initializing timeline...</div>;
+ return <div>Initializing timeline for {timelineName}...</div>;
}
return <Timeline name={timelineName} />;
- };
+ });Committable suggestion was skipped due to low confidence.
app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx
Outdated
Show resolved
Hide resolved
app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
app/packages/core/src/plugins/SchemaIO/components/FrameLoaderView.tsx (1)
Line range hint
1-99: Consider using React state management instead of custom events.The current implementation uses custom DOM events for communication between components. This approach:
- Makes the component tightly coupled to the browser's event system
- Makes testing more difficult
- Could lead to memory leaks if not properly cleaned up
Consider using React's built-in state management (Context, useState) or state management libraries (Recoil, as mentioned in guidelines) for better maintainability and testability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/packages/core/src/plugins/SchemaIO/components/FrameLoaderView.tsx (2 hunks)
- app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/core/src/plugins/SchemaIO/components/FrameLoaderView.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (2)
app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx (2)
5-31: Good implementation ofTimelineViewcomponentThe
TimelineViewcomponent correctly extracts and validates the required parameters, and passes the appropriate configuration toTimelineCreator.
37-37:⚠️ Potential issueFix reference to undefined variable 'total_frames'
The variable
total_framesis not defined in the scope ofTimelineCreator. You should usetotalFramesinstead.Apply this diff to fix the error:
- totalFrames: total_frames, + totalFrames: totalFrames,Likely invalid or redundant comment.
app/packages/core/src/plugins/SchemaIO/components/FrameLoaderView.tsx
Outdated
Show resolved
Hide resolved
app/packages/core/src/plugins/SchemaIO/components/FrameLoaderView.tsx
Outdated
Show resolved
Hide resolved
app/packages/core/src/plugins/SchemaIO/components/FrameLoaderView.tsx
Outdated
Show resolved
Hide resolved
app/packages/core/src/plugins/SchemaIO/components/FrameLoaderView.tsx
Outdated
Show resolved
Hide resolved
app/packages/core/src/plugins/SchemaIO/components/FrameLoaderView.tsx
Outdated
Show resolved
Hide resolved
app/packages/core/src/plugins/SchemaIO/components/FrameLoaderView.tsx
Outdated
Show resolved
Hide resolved
app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx
Outdated
Show resolved
Hide resolved
app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx
Outdated
Show resolved
Hide resolved
app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx
Outdated
Show resolved
Hide resolved
…iew.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
….tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ritch
force-pushed
the
feat-timeline-op-type
branch
from
0a62007
to
436c45c
Compare
|
lgtm! needs merge conflict resolved |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
fiftyone/operators/types.py (1)
2577-2589: Consider adding timeline-specific functionality.The timeline implementation could benefit from additional timeline-specific features:
- Frame rate control and synchronization
- Timeline markers for key events/frames
- Timeline segments for grouping frames
- Export/import timeline configurations
Example timeline configuration structure:
def __init__(self, **kwargs): super().__init__(**kwargs) self.timeline_name = kwargs.get("timeline_name", None) self.total_frames = kwargs.get("total_frames", None) self.loop = kwargs.get("loop", False) self.fps = kwargs.get("fps", 30) self.markers = kwargs.get("markers", {}) self.segments = kwargs.get("segments", []) def add_marker(self, frame, label): """Add a marker at a specific frame.""" self.markers[frame] = label def add_segment(self, start_frame, end_frame, name): """Add a named segment of frames.""" self.segments.append({ "name": name, "start": start_frame, "end": end_frame })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- app/packages/core/src/plugins/SchemaIO/components/FrameLoaderView.tsx (3 hunks)
- app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx (1 hunks)
- app/packages/core/src/plugins/SchemaIO/components/index.ts (1 hunks)
- fiftyone/operators/types.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/core/src/plugins/SchemaIO/components/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/core/src/plugins/SchemaIO/components/FrameLoaderView.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/plugins/SchemaIO/components/TimelineView.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (2)
app/packages/core/src/plugins/SchemaIO/components/FrameLoaderView.tsx (1)
10-11: LGTM! Good improvements to code maintainability.The addition of the
FRAME_LOADED_EVENTconstant and properly typedframeDataRefenhances code maintainability and type safety.Also applies to: 21-21
fiftyone/operators/types.py (1)
2577-2589: Past review comment is still applicable.The implementation issues identified in the previous review remain unaddressed.
| if (!isTimelineInitialized) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance loading state feedback
Returning null during initialization provides no feedback to users. Consider showing a loading indicator or message.
if (!isTimelineInitialized) {
- return null;
+ return <div aria-busy="true" role="status">Initializing timeline...</div>;
}📝 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.
| if (!isTimelineInitialized) { | |
| return null; | |
| } | |
| if (!isTimelineInitialized) { | |
| return <div aria-busy="true" role="status">Initializing timeline...</div>; | |
| } |
| if (data?.frames) frameDataRef.current = data.frames; | ||
| window.dispatchEvent( | ||
| new CustomEvent(FRAME_LOADED_EVENT, { | ||
| detail: { localId: localIdRef.current }, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Add missing dependency and cleanup to useEffect.
The effect should include data.frames in its dependency array and implement a cleanup function.
Apply this diff:
useEffect(() => {
localIdRef.current = Math.random().toString(36).substring(7);
if (data?.frames) frameDataRef.current = data.frames;
window.dispatchEvent(
new CustomEvent(FRAME_LOADED_EVENT, {
detail: { localId: localIdRef.current },
})
);
+ return () => {
+ frameDataRef.current = null;
+ };
-}, [data?.signature]);
+}, [data?.signature, data?.frames]);📝 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.
| if (data?.frames) frameDataRef.current = data.frames; | |
| window.dispatchEvent( | |
| new CustomEvent(FRAME_LOADED_EVENT, { | |
| detail: { localId: localIdRef.current }, | |
| }) | |
| ); | |
| useEffect(() => { | |
| localIdRef.current = Math.random().toString(36).substring(7); | |
| if (data?.frames) frameDataRef.current = data.frames; | |
| window.dispatchEvent( | |
| new CustomEvent(FRAME_LOADED_EVENT, { | |
| detail: { localId: localIdRef.current }, | |
| }) | |
| ); | |
| return () => { | |
| frameDataRef.current = null; | |
| }; | |
| }, [data?.signature, data?.frames]); |
| if (frameDataRef.current) { | ||
| bufm.current.addNewRange(range); | ||
| resolve(); | ||
| } else { | ||
| const onFramesLoaded = (e) => { | ||
| if ( | ||
| e instanceof CustomEvent && | ||
| e.detail.localId === localIdRef.current | ||
| ) { | ||
| window.removeEventListener(FRAME_LOADED_EVENT, onFramesLoaded); | ||
| bufm.current.addNewRange(range); | ||
| resolve(); | ||
| } | ||
| }; | ||
| window.addEventListener(FRAME_LOADED_EVENT, onFramesLoaded); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve event listener typing and cleanup pattern.
While the event listener cleanup is implemented, there are opportunities for improvement in typing and pattern usage.
Apply this diff for better type safety and a more robust cleanup pattern:
if (frameDataRef.current) {
bufm.current.addNewRange(range);
resolve();
} else {
- const onFramesLoaded = (e) => {
+ const onFramesLoaded = (e: CustomEvent<{ localId: string }>) => {
+ if (e.detail.localId !== localIdRef.current) return;
+
+ window.removeEventListener(FRAME_LOADED_EVENT, onFramesLoaded as EventListener);
+ bufm.current.addNewRange(range);
+ resolve();
+ };
- if (
- e instanceof CustomEvent &&
- e.detail.localId === localIdRef.current
- ) {
- window.removeEventListener(FRAME_LOADED_EVENT, onFramesLoaded);
- bufm.current.addNewRange(range);
- resolve();
- }
- };
window.addEventListener(FRAME_LOADED_EVENT, onFramesLoaded as EventListener);
}📝 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.
| if (frameDataRef.current) { | |
| bufm.current.addNewRange(range); | |
| resolve(); | |
| } else { | |
| const onFramesLoaded = (e) => { | |
| if ( | |
| e instanceof CustomEvent && | |
| e.detail.localId === localIdRef.current | |
| ) { | |
| window.removeEventListener(FRAME_LOADED_EVENT, onFramesLoaded); | |
| bufm.current.addNewRange(range); | |
| resolve(); | |
| } | |
| }; | |
| window.addEventListener(FRAME_LOADED_EVENT, onFramesLoaded); | |
| } | |
| if (frameDataRef.current) { | |
| bufm.current.addNewRange(range); | |
| resolve(); | |
| } else { | |
| const onFramesLoaded = (e: CustomEvent<{ localId: string }>) => { | |
| if (e.detail.localId !== localIdRef.current) return; | |
| window.removeEventListener(FRAME_LOADED_EVENT, onFramesLoaded as EventListener); | |
| bufm.current.addNewRange(range); | |
| resolve(); | |
| }; | |
| window.addEventListener(FRAME_LOADED_EVENT, onFramesLoaded as EventListener); | |
| } |
What changes are proposed in this pull request?
Adds a new
foo.types.TimelineView()for building custom animation.How is this patch tested? If it is not, please explain why.
With the
AnimatedExamplepanel.Summary by CodeRabbit
TimelineViewcomponent for rendering timelines based on provided schemas.TimelineCreatorcomponent to initialize and display timelines with dynamic configurations.PlotlyViewcomponent for better interaction responses.PlotlyView.FrameLoaderViewcomponent.PlotlyView.ModalViewandTextViewcomponents in the index file.