-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(timeline): add new timeline v2 component #6760
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to ac6ca85 in 3 minutes and 34 seconds
More details
- Looked at
232
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. frontend/src/components/TimelineV2/TimelineV2.tsx:27
- Draft comment:
The check forspread < 0
is redundant since the same condition is checked later withendTimestamp < startTimestamp
. Consider removing this check from theuseEffect
hook. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
While the checks are mathematically equivalent, they serve different purposes in different parts of the code. The useEffect check prevents unnecessary calculations and state updates, while the render check handles the error display. Having both checks provides clear separation of concerns - one for data processing, one for UI handling. Removing either could make the code less maintainable.
The comment assumes redundancy is always bad, but sometimes redundant checks in different layers can improve code clarity and maintainability. The checks also have different side effects (return vs error display).
The different purposes and side effects of these checks justify keeping both - they're not truly redundant as they serve distinct roles in the component's logic.
The comment should be deleted as it suggests a change that would make the code less clear by removing a useful layer of validation.
2. frontend/src/components/TimelineV2/TimelineV2.tsx:66
- Draft comment:
Consider using a more unique key generation strategy, such askey={index}
. The current method may lead to potential key collisions and performance issues. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is actually suggesting a worse practice. Using just index as a key is generally discouraged in React as it can lead to rendering issues when items are reordered or deleted. The current implementation combines multiple unique properties of each interval, which is more robust. The concatenation of percentage + label + index should provide sufficient uniqueness for React's reconciliation.
Could there be a better way to format the key string? Could string concatenation with + operator lead to unexpected results?
While the string concatenation could be formatted better, the fundamental approach of combining multiple unique properties is sound and better than using just index.
The comment should be deleted as it suggests a worse practice (using only index as key) than the current implementation.
3. frontend/src/components/TimelineV2/TimelineV2.tsx:61
- Draft comment:
The checkintervals && intervals.length > 0
is unnecessary.intervals.map
will handle empty arrays without issues. - Reason this comment was not posted:
Confidence changes required:50%
The use ofintervals && intervals.length > 0
is redundant sincemap
will handle empty arrays gracefully.
4. frontend/src/components/TimelineV2/utils.ts:98
- Draft comment:
Ensure thattempBaseSpread
is decremented correctly to avoid potential infinite loops. Consider adding a safety check or logging to monitor loop execution. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_Y2c9zbmHmrZHpEse
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 651dce1 in 37 seconds
More details
- Looked at
20
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. frontend/src/components/TimelineV2/TimelineV2.tsx:72
- Draft comment:
Consider extracting2 * Math.floor(timelineHeight / 4)
into a variable for clarity and to avoid repetition. - Reason this comment was not posted:
Confidence changes required:50%
The calculation fory
in the<text>
and<line>
elements is repeated and could be extracted into a variable for clarity and to avoid potential errors if the calculation needs to change.
2. frontend/src/components/TimelineV2/TimelineV2.tsx:78
- Draft comment:
Consider extracting3 * Math.floor(timelineHeight / 4)
into a variable for clarity and to avoid repetition. - Reason this comment was not posted:
Confidence changes required:50%
The calculation fory1
in the<line>
element is repeated and could be extracted into a variable for clarity and to avoid potential errors if the calculation needs to change.
3. frontend/src/components/TimelineV2/TimelineV2.tsx:73
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values forstroke
andfill
. This applies to lines 73 and 80. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_mtKeYc1AWCXWV8WS
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
* feat(timeline): base commit for timeline v2 * feat(timeline): svg rendering for timeline v2 * feat(timeline): dynamic scale based on screen size * feat(timeline): cleanup code * feat(timeline): make position functioning of timeline height
Summary
startTimestamp
,endTimestamp
decoupled from any particular module.Related Issues / PR's
contributes to - #6132
Screenshots
Screen.Recording.2025-01-07.at.1.11.54.PM.mov
Affected Areas and Manually Tested Areas