[WEB-2442] fix: Timeline Improvements and bug fixes#5922
Conversation
WalkthroughThe changes encompass several modifications across multiple files, primarily focusing on enhancing the functionality of timeline and Gantt chart components. Key updates include the introduction of a new method for adjusting block positions during drag operations, refinements to visibility detection in Gantt chart blocks, and adjustments to scroll handling logic. Additionally, the constructor parameter types for several stores have been updated to use Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
web/core/components/gantt-chart/helpers/blockResizables/right-resizable.tsx (1)
51-51: Consider extracting magic values to constants.
For better maintainability, consider extracting the transition duration and positioning values to named constants.
+const RESIZE_INDICATOR_TRANSITION_DURATION = 300; // milliseconds
+const RESIZE_INDICATOR_OFFSET = 1.5; // rem
+
<div
className={cn(
- "absolute right-1 top-1/2 -translate-y-1/2 h-7 w-1 z-[5] rounded-sm bg-custom-background-100 transition-all duration-300 opacity-0 group-hover:opacity-100",
+ `absolute right-1 top-1/2 -translate-y-1/2 h-7 w-1 z-[5] rounded-sm bg-custom-background-100 transition-all duration-${RESIZE_INDICATOR_TRANSITION_DURATION} opacity-0 group-hover:opacity-100`,
{
- "-right-1.5 opacity-100": isRightResizing,
+ [`-right-${RESIZE_INDICATOR_OFFSET} opacity-100`]: isRightResizing,
}
)}
/>web/core/hooks/use-auto-scroller.tsx (2)
Line range hint 57-83: Consider refactoring speed multiplier logic to reduce duplication.
The directional checks are well implemented, but the speed multiplier logic is repeated for each direction.
Consider extracting the speed multiplier logic into a helper function:
const getScrollSpeed = (position: number, threshold: number, maxSpeed: number, baseSpeed: number): number => {
if (position < maxSpeed) return baseSpeed * 2;
return baseSpeed;
};Then use it like:
- if (mouseX < thresholdX && diffX <= 0) {
- scrollByX = -1 * SCROLL_BY;
- if (mouseX < maxSpeedX) {
- scrollByX *= 2;
- }
- }
+ if (mouseX < thresholdX && diffX <= 0) {
+ scrollByX = -1 * getScrollSpeed(mouseX, thresholdX, maxSpeedX, SCROLL_BY);
+ }Line range hint 23-91: Consider adding error handling for edge cases.
While the auto-scroll logic is solid, consider adding try-catch blocks around critical sections to gracefully handle potential errors:
- Mouse event handling
- DOM operations (getBoundingClientRect, scrollBy)
- Event listener operations
Example implementation:
const handleAutoScroll = (e: MouseEvent) => {
try {
// existing logic
} catch (error) {
console.error('Auto-scroll error:', error);
clearRegisteredTimeout();
}
};web/core/components/gantt-chart/data/index.ts (1)
93-95: Consider documenting the rationale for the range value.
The comment explains what the value does but not why 24 was chosen as the optimal value. Consider adding a brief explanation of why this specific range was selected to help future maintainers understand the decision.
- approxFilterRange: 24, // it will preview week starting dates all months data and there is 3 months limitation for preview ex: title (2, 9, 16, 23, 30)
+ approxFilterRange: 24, // Shows 24 weeks of data (6 months) to ensure complete quarter visibility with buffer. Preview format: (2, 9, 16, 23, 30)web/core/components/gantt-chart/blocks/block-row.tsx (1)
Line range hint 33-63: Consider performance and type safety improvements.
The intersection observer setup could benefit from the following improvements:
- Memoize the observer callback using
useCallbackto prevent unnecessary re-renders - Add explicit type safety for the intersection observer root bounds
- Consider adding a small threshold/buffer zone for visibility detection to prevent flickering
Here's how you could improve the code:
+ import { useCallback } from "react";
useEffect(() => {
const intersectionRoot = document.querySelector("#gantt-container") as HTMLDivElement;
const timelineBlock = document.getElementById(`gantt-block-${block?.id}`);
if (!timelineBlock || !intersectionRoot) return;
setIsBlockHiddenOnLeft(
!!block.position?.marginLeft &&
!!block.position?.width &&
intersectionRoot.scrollLeft > block.position.marginLeft + block.position.width
);
+ const handleIntersection = useCallback((entries: IntersectionObserverEntry[]) => {
+ entries.forEach((entry) => {
+ const rootBounds = entry.rootBounds;
+ if (!rootBounds) return;
+
+ // Add a small buffer (e.g., 2px) to prevent flickering
+ const BUFFER = 2;
+ setIsHidden(!entry.isIntersecting);
+ setIsBlockHiddenOnLeft(entry.boundingClientRect.right < (rootBounds.left + BUFFER));
+ });
+ }, []);
const observer = new IntersectionObserver(
- (entries) => {
- entries.forEach((entry) => {
- setIsHidden(!entry.isIntersecting);
- setIsBlockHiddenOnLeft(entry.boundingClientRect.right < (entry.rootBounds?.left ?? 0));
- });
- },
+ handleIntersection,
{
root: intersectionRoot,
rootMargin: `0px 0px 0px -${SIDEBAR_WIDTH}px`,
}
);
observer.observe(timelineBlock);
return () => {
observer.unobserve(timelineBlock);
};
- }, [block]);
+ }, [block, handleIntersection]);web/core/components/gantt-chart/views/quarter-view.ts (1)
60-60: Consider adding documentation for the range halving logic.
The code correctly implements left-side expansion with range optimization, but the reasoning behind halving the range could be clearer to future maintainers.
Add a comment explaining the range optimization:
+ // Half the range for incremental loading to optimize performance
minusDate = new Date(currentDate.getFullYear(), currentDate.getMonth() - range / 2, 1);Also applies to: 66-67, 70-70
web/core/components/gantt-chart/helpers/blockResizables/use-gantt-resizable.ts (1)
Line range hint 115-117: Fix critical error handling bug.
The setToast function is called incorrectly without parentheses, causing errors to be silently ignored.
Apply this fix:
try {
const blockUpdates = getUpdatedPositionAfterDrag(block.id, dragDirection !== "move");
updateBlockDates && updateBlockDates(blockUpdates);
} catch (e) {
- setToast;
+ setToast({
+ message: "Failed to update block dates",
+ type: "error",
+ title: "Error"
+ });
}web/core/components/gantt-chart/views/week-view.ts (1)
49-51: Initialize dates with more specific values.
The initial values of startDate and endDate using new Date() are never used as they're immediately overwritten. Consider removing these initializations or setting them to null initially to make the code intent clearer.
- let startDate = new Date();
- let endDate = new Date();
+ let startDate: Date;
+ let endDate: Date;📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
- web/ce/store/timeline/base-timeline.store.ts (4 hunks)
- web/core/components/gantt-chart/blocks/block-row.tsx (1 hunks)
- web/core/components/gantt-chart/chart/main-content.tsx (1 hunks)
- web/core/components/gantt-chart/chart/root.tsx (4 hunks)
- web/core/components/gantt-chart/data/index.ts (1 hunks)
- web/core/components/gantt-chart/helpers/blockResizables/left-resizable.tsx (1 hunks)
- web/core/components/gantt-chart/helpers/blockResizables/right-resizable.tsx (1 hunks)
- web/core/components/gantt-chart/helpers/blockResizables/use-gantt-resizable.ts (1 hunks)
- web/core/components/gantt-chart/views/helpers.ts (0 hunks)
- web/core/components/gantt-chart/views/month-view.ts (5 hunks)
- web/core/components/gantt-chart/views/quarter-view.ts (2 hunks)
- web/core/components/gantt-chart/views/week-view.ts (5 hunks)
- web/core/hooks/use-auto-scroller.tsx (4 hunks)
- web/core/store/timeline/issues-timeline.store.ts (2 hunks)
- web/core/store/timeline/modules-timeline.store.ts (2 hunks)
💤 Files with no reviewable changes (1)
- web/core/components/gantt-chart/views/helpers.ts
🔇 Additional comments (28)
web/core/store/timeline/modules-timeline.store.ts (2)
13-13: LGTM: Constructor signature updated consistently.
The constructor parameter type change maintains consistency with the base class while preserving the existing initialization logic and autorun behavior.
3-3: LGTM: Import change aligns with store hierarchy.
The change from CoreRootStore to RootStore appears to be part of a broader refactoring effort to standardize store implementations.
Let's verify this is part of a consistent pattern across other timeline stores:
✅ Verification successful
Import change is consistent with timeline store implementations
The verification confirms that:
- Both
modules-timeline.store.tsandissues-timeline.store.tsconsistently useRootStorefrom "@/plane-web/store/root.store" - The constructor parameter type
RootStoreis consistently used across timeline stores - The base timeline store in CE also uses the same
RootStoreimport pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the RootStore usage pattern across timeline stores
# Expected: All timeline stores should use RootStore consistently
# Search for timeline store definitions and their constructor parameters
ast-grep --pattern 'export class $_TimeLineStore extends BaseTimeLineStore {
$$$
constructor($_: RootStore) {
$$$
}
}'
Length of output: 130
Script:
#!/bin/bash
# Let's try a broader search to understand the timeline store implementations
# First, find all timeline store files
echo "=== Timeline Store Files ==="
fd timeline.store
# Then search for RootStore imports
echo -e "\n=== RootStore Imports ==="
rg "import.*RootStore.*from" --type ts
# Look for constructor patterns in timeline stores
echo -e "\n=== Timeline Store Constructors ==="
rg "constructor.*\(.*\)" --type ts $(fd timeline.store)
# Check BaseTimeLineStore usage
echo -e "\n=== BaseTimeLineStore Usage ==="
rg "extends BaseTimeLineStore" --type ts
Length of output: 9932
web/core/store/timeline/issues-timeline.store.ts (2)
Line range hint 13-20: Verify type compatibility between RootStore and CoreRootStore.
The constructor parameter type change from CoreRootStore to RootStore needs verification to ensure type compatibility, especially for the getIssueById method used in the autorun.
#!/bin/bash
# Description: Verify RootStore type definition and compatibility
# Check RootStore definition and its relationship with CoreRootStore
ast-grep --pattern 'class RootStore {
$$$
}'
# Check if RootStore implements or extends CoreRootStore
ast-grep --pattern 'class RootStore extends $_ {
$$$
}'3-3: Verify consistent import path usage across the codebase.
The import path change from @/store/root.store to @/plane-web/store/root.store suggests an architectural reorganization.
✅ Verification successful
Let me gather more information to verify the import paths comprehensively.
Import path changes are part of a consistent architectural update
The verification shows that timeline-related stores (issues-timeline.store.ts, modules-timeline.store.ts, and base-timeline.store.ts) consistently use the new import path @/plane-web/store/root.store. This change aligns with the architectural reorganization where timeline components are being moved to the plane-web structure.
Other files in the codebase still use different import paths (@/store/root.store, ../root.store) as they haven't been part of this architectural update yet. This is expected as the PR focuses specifically on timeline improvements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining imports from the old path
# and verify consistent usage of the new path
echo "Checking for old import path usage..."
rg "from ['\"]\@\/store\/root\.store['\"]"
echo "Verifying new import path usage..."
rg "from ['\"]\@\/plane-web\/store\/root\.store['\"]"
Length of output: 1194
Script:
#!/bin/bash
# Check all imports related to RootStore across the codebase
echo "Checking all RootStore imports..."
rg "import.*RootStore.*from.*" -A 1
# Check specifically for timeline-related stores importing RootStore
echo -e "\nChecking timeline stores..."
fd "timeline.*store" --exec rg "import.*RootStore.*from.*" {} -A 1
Length of output: 14673
web/core/components/gantt-chart/helpers/blockResizables/left-resizable.tsx (1)
53-56: Great UX improvements to the resize handle visibility!
The changes enhance the visual feedback during timeline interactions by:
- Hiding the resize handle by default to reduce visual clutter
- Smoothly revealing it on hover with a 300ms transition
- Ensuring it remains visible during active resizing operations
This implementation aligns well with modern UI patterns and improves the overall user experience.
web/core/components/gantt-chart/helpers/blockResizables/right-resizable.tsx (1)
51-54: LGTM! Enhanced visual feedback for resize indicators.
The changes improve the visibility of drag indicators through:
- Smooth opacity transitions (0 to 100) on hover
- Consistent visibility during active resizing
- Better positioning during drag operations
This aligns well with the PR objectives for improving drag indicators visibility on blocks.
web/core/hooks/use-auto-scroller.tsx (2)
3-3: Verify if the increased scroll speed provides smooth user experience.
The scroll increment has been tripled from 1 to 3. While this could improve responsiveness, it might affect precision during drag operations.
Please test the following scenarios to ensure smooth scrolling:
- Dragging small blocks near viewport edges
- Dragging during timeline zoom operations
- Performance on lower-end devices
Line range hint 16-38: LGTM! Improved scroll direction detection.
The addition of mouse position tracking helps prevent unwanted scrolling by considering the direction of mouse movement. This is a good improvement that aligns with the PR objective of enhancing auto-scroll logic.
web/core/components/gantt-chart/data/index.ts (1)
93-93: Verify performance impact of increased quarter view range.
The increase in approxFilterRange from 18 to 24 weeks provides better visibility of timeline data in the quarter view. However, this 33% increase in the preview range could impact rendering performance.
Let's verify the performance implications:
✅ Verification successful
Let me gather more specific information about the Gantt chart implementation and its rendering optimizations.
Based on the analysis of the codebase, I can now provide a final response regarding the performance impact of increasing the approxFilterRange.
Increasing quarter view range to 24 is safe for performance
The implementation shows that the approxFilterRange is used efficiently in the view generation logic:
- The range only affects the initial calculation of date ranges and filtered dates
- Data rendering is optimized through
RenderIfVisibleHOC which implements virtualization - The scrollWidth calculation is independent of the range size
- Each view (week, month, quarter) has its own optimized range handling
The increase from 18 to 24 in quarter view maintains consistency with the existing pattern where:
- Week view uses range of 6
- Month view uses range of 6
- Quarter view uses larger ranges (previously 18, now 24) to accommodate its broader time scope
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for performance-related configurations and optimizations in timeline components
# Test 1: Look for existing performance optimizations or virtual scrolling implementations
echo "Searching for virtual scrolling or performance optimization patterns..."
rg -i "virtualiz|windowing|performance" --type ts
# Test 2: Check for render-related hooks or components
echo "Checking render optimization patterns..."
ast-grep --pattern 'useCallback|useMemo|memo|shouldComponentUpdate'
Length of output: 1661
Script:
#!/bin/bash
# Search for Gantt chart rendering and data handling implementations
echo "Checking Gantt chart rendering patterns..."
rg -A 5 "render|draw|paint" web/core/components/gantt-chart/
# Look for existing timeline data handling and optimizations
echo "Checking timeline data handling..."
rg -A 5 "(timeline|range).*(data|items|render)" web/core/components/gantt-chart/
# Check for any existing memoization or optimization in the Gantt component
echo "Checking optimization patterns in Gantt component..."
rg -A 5 "(useMemo|useCallback|React\.memo)" web/core/components/gantt-chart/
Length of output: 27706
web/core/components/gantt-chart/blocks/block-row.tsx (1)
51-51: Improved block visibility detection logic.
The new condition entry.boundingClientRect.right < (entry.rootBounds?.left ?? 0) is more accurate for determining when a block is hidden on the left side. It ensures the block is completely out of view (right edge is less than the viewport's left edge) before showing the navigation arrow.
Let's verify the arrow direction behavior:
✅ Verification successful
Block visibility logic is correctly implemented and consistent
The verification confirms that:
- The new block visibility logic in
block-row.tsxis the only implementation ofsetIsBlockHiddenOnLeftin the codebase - Arrow rotation behavior (
rotate-180) is consistently implemented across the codebase for directional indicators - The change from checking left edge to checking right edge against viewport bounds is an isolated improvement that doesn't conflict with other implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any related arrow direction logic in the codebase
# to ensure consistent behavior across timeline views
# Test 1: Look for other files that might have similar arrow direction logic
rg -A 5 "ArrowRight.*rotate-180"
# Test 2: Look for related timeline visibility checks
ast-grep --pattern 'setIsBlockHiddenOnLeft($$$)'
Length of output: 2475
web/core/components/gantt-chart/views/quarter-view.ts (4)
30-32: LGTM: Clean variable initialization.
The early initialization of startDate and endDate variables helps maintain consistent state throughout the function.
78-78: LGTM: Symmetric implementation with left-side handling.
The right-side expansion logic correctly mirrors the left-side approach, maintaining consistency in the codebase.
Also applies to: 83-84, 87-87
91-91: Important: Inclusive date range calculation fix.
The addition of + 1 in the days calculation is crucial as it ensures:
- The end date is included in the range
- Correct scrollWidth calculation for the timeline
- Proper block positioning and visibility
This change directly addresses the PR objective of improving timeline layout.
Let's verify this fix is consistently applied:
✅ Verification successful
Inclusive date range calculation is consistently implemented across all views
The verification confirms that the + 1 adjustment for inclusive date range calculation is properly applied in all relevant view files:
month-view.tsweek-view.tsquarter-view.ts
This consistency ensures uniform behavior across different timeline views.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the inclusive date calculation is consistent across views
# Expect: Similar adjustments in other view calculations
rg "getNumberOfDaysBetweenTwoDates.*\+ 1" "web/core/components/gantt-chart/views/"
Length of output: 482
44-52: LGTM: Proper date range initialization.
The date range is correctly calculated:
- Start date is set to the first day of the first month
- End date is set to the last day of the last month
- State updates maintain immutability
Let's verify the date calculations are consistent with other views:
✅ Verification successful
Let me gather more specific information about the date initialization patterns across views.
Date initialization is consistent across views
The date handling in the quarter view aligns with the patterns in week and month views:
- All views initialize start/end dates with
new Date() - When
side === null, each view calculates dates symmetrically around the current date - The quarter view's approach of using
month + 1and day0to get the last day of the month is consistent with the date arithmetic used in other views - All views properly update the renderState with immutable patterns
- Each view calculates the scroll width using the same formula:
days * viewPayload.data.dayWidth
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check date handling consistency across views
# Expect: Similar date initialization patterns in month and week views
rg -A 5 "startDate.*=.*new Date" "web/core/components/gantt-chart/views/"
Length of output: 482
Script:
#!/bin/bash
# Search for date initialization patterns in all view files
rg -A 10 "startDate.*=.*new Date|endDate.*=.*new Date" "web/core/components/gantt-chart/views/"
Length of output: 10854
web/core/components/gantt-chart/helpers/blockResizables/use-gantt-resizable.ts (1)
77-78: Verify the left-drag width calculation fix.
The new width calculation logic using marginDelta aligns with the PR objective to fix block width during left-handle dragging. However, we should verify this fix works correctly across different scenarios.
web/core/components/gantt-chart/views/month-view.ts (5)
6-6: LGTM: Good use of utility function.
The addition of getNumberOfDaysBetweenTwoDates import promotes code reuse and standardization of date calculations across the codebase.
41-43: LGTM: Clear variable initialization.
The introduction of startDate and endDate variables improves code clarity by explicitly tracking the date range used for calculations.
97-98: LGTM: Improved scrollWidth calculation accuracy.
The new calculation using getNumberOfDaysBetweenTwoDates provides more precise results by counting actual days rather than weeks. The +1 correctly includes both start and end dates in the calculation.
89-93:
Fix null safety and remove redundant endDate assignment.
The code has null safety concerns and contains redundant endDate assignment in renderState.
Apply this diff to fix both issues:
- startDate = new Date(currentDate.getFullYear(), currentDate.getMonth(), currentDate.getDate() + 1);
- endDate = filteredDates.weeks[filteredDates.weeks.length - 1]?.endDate;
+ startDate = minusDate;
+ endDate = filteredDates.weeks[filteredDates.weeks.length - 1]?.endDate ?? plusDate;
renderState = {
...renderState,
- data: { ...renderState.data, endDate: filteredDates.weeks[filteredDates.weeks.length - 1]?.endDate },
+ data: { ...renderState.data, endDate },
};Likely invalid or redundant comment.
73-77:
Add null safety check and verify date calculations.
Similar null safety concern exists here. Additionally, let's verify the date calculations are working as expected.
Apply this diff to add null safety:
- startDate = filteredDates.weeks[0]?.startDate;
+ startDate = filteredDates.weeks[0]?.startDate ?? minusDate;web/core/components/gantt-chart/views/week-view.ts (4)
4-4: LGTM: Improved date calculation approach.
The addition of getNumberOfDaysBetweenTwoDates import enables more precise date difference calculations.
61-68: LGTM: Improved date synchronization logic.
The date handling logic has been improved across all scroll cases:
- Null case: Properly syncs both start and end dates
- Left case: Maintains continuity with current view's start date
- Right case: Maintains continuity with current view's end date
Also applies to: 81-85, 97-101
105-106: LGTM: More accurate scroll width calculation.
The new scroll width calculation using getNumberOfDaysBetweenTwoDates is more precise and reliable than the previous array length-based approach. Adding 1 to the days ensures inclusive date ranges.
105-106: Verify integration with drag operations.
Since this change affects scroll width calculations, please verify that it properly integrates with block positioning during drag operations, especially when dragging from the left handle (mentioned in PR objectives).
✅ Verification successful
Let me try different commands to gather more information about drag operations and width calculations.
Let me check the specific implementation of width calculations and block positioning during drag operations.
Based on the results, I can now generate the final response since we have sufficient evidence about how the scroll width and drag operations work together.
The scroll width calculation is safe for drag operations
The scroll width calculation (days * weekPayload.data.dayWidth) is properly integrated with the drag operations:
- The drag implementation in
use-gantt-resizable.tsuses the samedayWidthunit for calculations, ensuring consistency - Block width and position during drag are calculated using
Math.round(mouseX / dayWidth) * dayWidth, which maintains alignment with the timeline grid - The scroll width serves as the container boundary while the drag operations snap to the same day-width grid
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for drag-related code that might be affected by these changes
echo "Searching for drag-related code in timeline components..."
rg -l "drag" --type ts --type tsx web/core/components/gantt-chart/
echo "Searching for specific block width calculations during drag..."
ast-grep --pattern 'width = $_ * $_' web/core/components/gantt-chart/
Length of output: 426
Script:
#!/bin/bash
# Search for drag-related code with proper file extension
echo "Searching for drag-related code..."
rg -l "drag" web/core/components/gantt-chart/
# Look for width calculations and timeline positioning
echo -e "\nSearching for width calculations..."
rg "width.*=.*dayWidth" web/core/components/gantt-chart/
# Check for block positioning logic
echo -e "\nSearching for block positioning code..."
rg "position.*block" -A 3 web/core/components/gantt-chart/
# Look for handle-related drag code
echo -e "\nSearching for handle drag code..."
rg "handle.*drag|drag.*handle" -A 3 web/core/components/gantt-chart/
Length of output: 6145
Script:
#!/bin/bash
# Check the implementation of use-gantt-resizable
echo "Checking resizable implementation..."
rg -A 10 "width = Math\.round" web/core/components/gantt-chart/helpers/blockResizables/use-gantt-resizable.ts
# Look at the week view implementation around the scroll width calculation
echo -e "\nChecking week view context..."
rg -A 10 -B 5 "scrollWidth = days \* weekPayload" web/core/components/gantt-chart/views/week-view.ts
# Check how block position is calculated
echo -e "\nChecking block position calculation..."
rg -A 10 "getBlockPositionAndWidth" web/core/components/gantt-chart/views/helpers.ts
Length of output: 1475
web/core/components/gantt-chart/chart/root.tsx (2)
13-21: LGTM! Well-structured view implementation mapping.
The reorganization of timeline view helpers into a centralized mapping improves code maintainability and provides a cleaner interface for accessing view-specific logic.
Also applies to: 46-50
79-87: Verify block position updates during drag operations.
The addition of updateAllBlocksOnChartChangeWhileDragging appears to address the block width fix mentioned in the PR objectives. Let's verify its implementation:
✅ Verification successful
Let me gather more context about the implementation by checking the full store implementation and drag-related code.
Let me check one final aspect - how the block updates are handled during dragging to verify the complete implementation.
Based on the gathered evidence, I can now provide the final response:
Implementation of block position updates during drag operations is properly handled
The updateAllBlocksOnChartChangeWhileDragging method is correctly implemented with proper safeguards:
- Only executes when blocks exist and dragging is active (
isDraggingflag) - Updates block positions by adjusting
marginLeftfor all blocks during chart changes - Drag state is properly managed through
setIsDraggingin drag lifecycle events - The implementation aligns with the PR's objective of maintaining correct block positions during drag operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of updateAllBlocksOnChartChangeWhileDragging
# Look for related test coverage and usage
# Check for test coverage
echo "Checking for test coverage..."
rg -l "updateAllBlocksOnChartChangeWhileDragging" --glob "**/*.test.{ts,tsx}"
# Check for other usages to understand the full context
echo "Checking for all usages..."
rg "updateAllBlocksOnChartChangeWhileDragging" -A 3
Length of output: 2097
Script:
#!/bin/bash
# Check the full implementation context
echo "Checking store implementation..."
ast-grep --pattern 'class $_ {
$$$
isDragging = $_;
$$$
updateAllBlocksOnChartChangeWhileDragging = $_($$$) {
$$$
}
$$$
}'
echo "Checking drag-related code..."
rg "isDragging" -B 2 -A 2 web/ce/store/timeline/base-timeline.store.ts
Length of output: 1672
Script:
#!/bin/bash
# Check the block update implementation during drag
echo "Checking block updates during drag..."
rg -A 10 "updateAllBlocksOnChartChangeWhileDragging = action" web/ce/store/timeline/base-timeline.store.ts
# Check where the drag state is modified
echo "Checking drag state modifications..."
rg "setIsDragging\(" -A 5 -B 1
Length of output: 14288
web/ce/store/timeline/base-timeline.store.ts (2)
12-12: LGTM: Interface updates align with timeline improvements.
The addition of updateAllBlocksOnChartChangeWhileDragging to the interface properly declares the new functionality for handling block position updates during chart changes.
Also applies to: 41-41
61-61: Verify the impact of RootStore type change.
The constructor parameter type has been changed from CoreRootStore to RootStore. While this aligns with the updated import, we should ensure all derived classes are compatible with this change.
Also applies to: 63-63
✅ Verification successful
Type change from CoreRootStore to RootStore is safe and correct
The verification shows that:
- Both derived classes (
ModulesTimeLineStoreandIssuesTimeLineStore) already useRootStoretype in their constructors RootStoreextendsCoreRootStore(found in web/ce/store/root.store.ts), making this a type-safe change- The change maintains the type hierarchy and is consistent across the timeline stores
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for classes extending BaseTimeLineStore to verify type compatibility
ast-grep --pattern 'class $_ extends BaseTimeLineStore'
Length of output: 57
Script:
#!/bin/bash
# Let's try a different approach to find classes extending BaseTimeLineStore
# First, let's find files containing "BaseTimeLineStore"
rg "BaseTimeLineStore" -l
# Then let's look for class definitions and extensions in those files
rg "class.*extends.*BaseTimeLineStore" -A 5
Length of output: 1189
Script:
#!/bin/bash
# Let's also check the base class implementation to understand the constructor
rg "constructor.*\(.*RootStore" -A 3 web/ce/store/timeline/base-timeline.store.ts
Length of output: 199
Script:
#!/bin/bash
# Let's check for any usage of CoreRootStore type
rg "CoreRootStore"
Length of output: 10198
| // get Dimensions from dom's style | ||
| const prevMarginLeft = parseFloat(resizableDiv.style.transform.slice(11, -3)); | ||
| const prevWidth = parseFloat(resizableDiv.style.width.slice(0, -2)); | ||
| // calculate new width | ||
| width = blockRight - marginLeft; | ||
| const marginDelta = prevMarginLeft - marginLeft; | ||
| width = prevWidth + marginDelta; |
There was a problem hiding this comment.
Improve robustness of dimension retrieval logic.
The current implementation has potential fragility issues:
- Direct style string parsing with magic numbers (
slice(11, -3)andslice(0, -2)) is brittle - No error handling for missing or malformed style properties
Consider this safer implementation:
- const prevMarginLeft = parseFloat(resizableDiv.style.transform.slice(11, -3));
- const prevWidth = parseFloat(resizableDiv.style.width.slice(0, -2));
+ // Extract translateX value using regex
+ const transformMatch = resizableDiv.style.transform.match(/translateX\(([-\d.]+)px\)/);
+ const prevMarginLeft = transformMatch ? parseFloat(transformMatch[1]) : 0;
+
+ // Safely parse width
+ const prevWidth = resizableDiv.style.width ? parseFloat(resizableDiv.style.width) : 0;
+
+ // Validate parsed values
+ if (isNaN(prevMarginLeft) || isNaN(prevWidth)) {
+ console.warn('Invalid block dimensions detected');
+ return;
+ }📝 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.
| // get Dimensions from dom's style | |
| const prevMarginLeft = parseFloat(resizableDiv.style.transform.slice(11, -3)); | |
| const prevWidth = parseFloat(resizableDiv.style.width.slice(0, -2)); | |
| // calculate new width | |
| width = blockRight - marginLeft; | |
| const marginDelta = prevMarginLeft - marginLeft; | |
| width = prevWidth + marginDelta; | |
| // get Dimensions from dom's style | |
| // Extract translateX value using regex | |
| const transformMatch = resizableDiv.style.transform.match(/translateX\(([-\d.]+)px\)/); | |
| const prevMarginLeft = transformMatch ? parseFloat(transformMatch[1]) : 0; | |
| // Safely parse width | |
| const prevWidth = resizableDiv.style.width ? parseFloat(resizableDiv.style.width) : 0; | |
| // Validate parsed values | |
| if (isNaN(prevMarginLeft) || isNaN(prevWidth)) { | |
| console.warn('Invalid block dimensions detected'); | |
| return; | |
| } | |
| // calculate new width | |
| const marginDelta = prevMarginLeft - marginLeft; | |
| width = prevWidth + marginDelta; |
| startDate = filteredDates.weeks[0]?.startDate; | ||
| endDate = filteredDates.weeks[filteredDates.weeks.length - 1]?.endDate; | ||
| renderState = { | ||
| ...renderState, | ||
| data: { | ||
| ...renderState.data, | ||
| startDate: filteredDates.weeks[0]?.startDate, | ||
| endDate: filteredDates.weeks[filteredDates.weeks.length - 1]?.endDate, | ||
| startDate, | ||
| endDate, |
There was a problem hiding this comment.
Add null safety checks for weeks array access.
The code assumes filteredDates.weeks array is not empty. Consider adding null safety checks to prevent potential runtime errors.
Apply this diff to add null safety:
- startDate = filteredDates.weeks[0]?.startDate;
- endDate = filteredDates.weeks[filteredDates.weeks.length - 1]?.endDate;
+ startDate = filteredDates.weeks[0]?.startDate ?? minusDate;
+ endDate = filteredDates.weeks[filteredDates.weeks.length - 1]?.endDate ?? plusDate;Committable suggestion was skipped due to low confidence.
| const approxRangeLeft = scrollLeft; | ||
| const approxRangeRight = scrollWidth - (scrollLeft + clientWidth); | ||
|
|
||
| if (approxRangeRight < 1000) updateCurrentViewRenderPayload("right", currentView); | ||
| if (approxRangeLeft < 1000) updateCurrentViewRenderPayload("left", currentView); | ||
| if (approxRangeRight < clientWidth) updateCurrentViewRenderPayload("right", currentView); | ||
| if (approxRangeLeft < clientWidth) updateCurrentViewRenderPayload("left", currentView); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve scroll handling readability and maintainability.
The scroll handling logic improvements are good - using clientWidth as a dynamic threshold is more responsive and adaptable than a fixed value. However, we can make it even better:
Consider these improvements:
- const approxRangeLeft = scrollLeft;
- const approxRangeRight = scrollWidth - (scrollLeft + clientWidth);
+ const distanceFromStart = scrollLeft;
+ const distanceFromEnd = scrollWidth - (scrollLeft + clientWidth);
+ const LOAD_THRESHOLD = clientWidth; // pixels before edge to trigger load
- if (approxRangeRight < clientWidth) updateCurrentViewRenderPayload("right", currentView);
- if (approxRangeLeft < clientWidth) updateCurrentViewRenderPayload("left", currentView);
+ if (distanceFromEnd < LOAD_THRESHOLD) updateCurrentViewRenderPayload("right", currentView);
+ if (distanceFromStart < LOAD_THRESHOLD) updateCurrentViewRenderPayload("left", currentView);This refactor:
- Uses more descriptive variable names that better explain their purpose
- Extracts the threshold to a named constant for better maintainability
- Maintains the improved dynamic threshold behavior
📝 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.
| const approxRangeLeft = scrollLeft; | |
| const approxRangeRight = scrollWidth - (scrollLeft + clientWidth); | |
| if (approxRangeRight < 1000) updateCurrentViewRenderPayload("right", currentView); | |
| if (approxRangeLeft < 1000) updateCurrentViewRenderPayload("left", currentView); | |
| if (approxRangeRight < clientWidth) updateCurrentViewRenderPayload("right", currentView); | |
| if (approxRangeLeft < clientWidth) updateCurrentViewRenderPayload("left", currentView); | |
| const distanceFromStart = scrollLeft; | |
| const distanceFromEnd = scrollWidth - (scrollLeft + clientWidth); | |
| const LOAD_THRESHOLD = clientWidth; // pixels before edge to trigger load | |
| if (distanceFromEnd < LOAD_THRESHOLD) updateCurrentViewRenderPayload("right", currentView); | |
| if (distanceFromStart < LOAD_THRESHOLD) updateCurrentViewRenderPayload("left", currentView); |
| updateCurrentView(selectedCurrentView); | ||
| updateRenderView(mergeRenderPayloads(currentRender.payload, renderView)); | ||
| updatingCurrentLeftScrollPosition(currentRender.scrollWidth); | ||
| updateAllBlocksOnChartChangeWhileDragging(currentRender.scrollWidth); |
There was a problem hiding this comment.
Consider updating blocks for both scroll directions.
The updateAllBlocksOnChartChangeWhileDragging is only called during left scroll operations. This asymmetric behavior might lead to inconsistent block positioning when scrolling right.
Consider applying the same update for right scroll operations:
} else if (side === "right") {
updateCurrentView(view);
updateRenderView(mergeRenderPayloads(renderView, currentRender.payload));
+ updateAllBlocksOnChartChangeWhileDragging(currentRender.scrollWidth);
setItemsContainerWidth(itemsContainerWidth + currentRender.scrollWidth);Committable suggestion was skipped due to low confidence.
| /** | ||
| * Adds width on Chart position change while the blocks are being dragged | ||
| * @param addedWidth | ||
| */ | ||
| updateAllBlocksOnChartChangeWhileDragging = action((addedWidth: number) => { | ||
| if (!this.blockIds || !this.isDragging) return; | ||
|
|
||
| runInAction(() => { | ||
| this.blockIds?.forEach((blockId) => { | ||
| const currBlock = this.blocksMap[blockId]; | ||
|
|
||
| if (!currBlock || !currBlock.position) return; | ||
|
|
||
| currBlock.position.marginLeft += addedWidth; | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation and optimizing performance.
The new method implementation has several areas for improvement:
- Input validation: Consider validating the
addedWidthparameter. - Error handling: Add error handling for invalid block positions.
- Performance: Consider updating only visible blocks instead of all blocks.
Here's a suggested implementation with these improvements:
updateAllBlocksOnChartChangeWhileDragging = action((addedWidth: number) => {
+ if (typeof addedWidth !== 'number' || isNaN(addedWidth)) {
+ console.warn('Invalid width adjustment provided');
+ return;
+ }
if (!this.blockIds || !this.isDragging) return;
runInAction(() => {
- this.blockIds?.forEach((blockId) => {
+ // Optional: Filter visible blocks first
+ const visibleBlocks = this.blockIds?.filter((blockId) => {
+ const block = this.blocksMap[blockId];
+ return block?.position?.marginLeft !== undefined;
+ });
+
+ visibleBlocks?.forEach((blockId) => {
const currBlock = this.blocksMap[blockId];
if (!currBlock || !currBlock.position) return;
+ try {
currBlock.position.marginLeft += addedWidth;
+ } catch (error) {
+ console.error(`Failed to update position for block ${blockId}:`, error);
+ }
});
});
});Committable suggestion was skipped due to low confidence.
3 participants
This PR focuses on the Bug fixes and minor improvements in timeline layout. They include,
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Chores