[WEB-2442] fix : Timeline layout bugs#5946
Conversation
… instead of directly importing it
WalkthroughThe changes in this pull request primarily focus on enhancing the handling of issue relations across various components and methods. Modifications include the introduction of new functions, updates to existing method signatures, and the integration of a dynamic approach for accessing relation options through the Changes
Possibly related PRs
Suggested labels
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
CodeRabbit Configuration File (
|
rahulramesha
changed the title
![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 (25)
web/ce/components/relations/activity.ts (2)
3-4: Consider making the null check more explicit.While the current check works, being more explicit about the check would improve code readability and maintainability.
- if (!activity) return; + if (activity === undefined) return undefined;
3-20: Add unit tests for relation activity content generation.This utility function is used by multiple components and handles critical relation state messages. Consider adding unit tests to verify all cases and edge conditions.
Would you like me to help create comprehensive unit tests for this function? I can generate test cases covering:
- All relation types
- Empty and non-empty old_value scenarios
- Undefined activity handling
web/core/components/issues/issue-detail/issue-activity/activity/actions/relation.tsx (1)
21-22: Add error handling for activity content generation.While the code is more maintainable now, consider adding error handling for the
getRelationActivityContentcall to gracefully handle edge cases.- const activityContent = getRelationActivityContent(activity); + const activityContent = activity ? getRelationActivityContent(activity) : null;web/ce/components/relations/index.tsx (1)
38-39: Consider memoizing the relation options.While the implementation is correct, consider memoizing the return value to prevent unnecessary re-renders in components that depend on these options.
-export const useTimeLineRelationOptions = () => ISSUE_RELATION_OPTIONS; +import { useMemo } from 'react'; + +export const useTimeLineRelationOptions = () => { + return useMemo(() => ISSUE_RELATION_OPTIONS, []); +};web/core/components/issues/issue-detail-widgets/relations/quick-action-button.tsx (1)
23-23: Consider adding error handling for the hook.While the hook usage is correct, consider handling potential initialization errors or loading states to improve robustness.
- const ISSUE_RELATION_OPTIONS = useTimeLineRelationOptions(); + const { relationOptions: ISSUE_RELATION_OPTIONS, error, isLoading } = useTimeLineRelationOptions(); + + if (error) { + return <div>Failed to load relation options</div>; + } + + if (isLoading) { + return <div>Loading...</div>; + }web/core/components/gantt-chart/blocks/block-row-list.tsx (2)
13-14: Consider adding JSDoc comments for the new prop.The type definition is correct, but adding documentation would improve maintainability.
export type GanttChartBlocksProps = { blockUpdateHandler: (block: any, payload: IBlockUpdateData) => void; + /** Callback to handle scrolling to a specific block in the Gantt chart */ handleScrollToBlock: (block: IGanttBlock) => void;
Line range hint
35-54: Fix the key prop placement for proper list rendering.The
keyprop is currently placed on theBlockRowcomponent, but it's wrapped in a fragment andRenderIfVisible. This could lead to React reconciliation issues.Move the key prop to the outermost element:
{blockIds?.map((blockId) => ( - <> + <React.Fragment key={blockId}> <RenderIfVisible root={ganttContainerRef} horizontalOffset={100} verticalOffset={200} classNames="relative min-w-full w-max" placeholderChildren={<div className="w-full pointer-events-none" style={{ height: `${BLOCK_HEIGHT}px` }} />} shouldRecordHeights={false} > <BlockRow - key={blockId} blockId={blockId} showAllBlocks={showAllBlocks} blockUpdateHandler={blockUpdateHandler} handleScrollToBlock={handleScrollToBlock} enableAddBlock={typeof enableAddBlock === "function" ? enableAddBlock(blockId) : enableAddBlock} selectionHelpers={selectionHelpers} ganttContainerRef={ganttContainerRef} /> </RenderIfVisible> - </> + </React.Fragment>web/core/components/issues/issue-detail/issue-activity/activity/activity-list.tsx (1)
44-45: Improve variable naming conventionThe variable
ISSUE_RELATION_OPTIONSuses uppercase naming convention typically reserved for static constants, but it's actually a dynamic value from a hook. Consider using camelCase to better reflect its nature.- const ISSUE_RELATION_OPTIONS = useTimeLineRelationOptions(); + const issueRelationOptions = useTimeLineRelationOptions(); - const activityRelations = getValidKeysFromObject(ISSUE_RELATION_OPTIONS); + const activityRelations = getValidKeysFromObject(issueRelationOptions);web/helpers/array.helper.ts (2)
95-99: Enhance type safety and documentation.The JSDoc comments could be more descriptive with examples and the types could be more specific.
/** * Returns Valid keys from object whose value is not falsy + * @param obj - The object to extract keys from + * @returns Array of keys whose corresponding values are truthy + * @example + * getValidKeysFromObject({ a: 1, b: 0, c: true, d: '' }) + * // returns ['a', 'c'] */ -export const getValidKeysFromObject = (obj: any) => { +export const getValidKeysFromObject = <T extends Record<string, unknown>>(obj: T): (keyof T)[] => {
94-104: Good architectural fit with Timeline relation processing.This utility function aligns well with the PR objectives for handling Timeline relations. Its placement in the array helpers makes sense as it complements existing object/array manipulation utilities.
Consider creating a dedicated test file for this function to ensure it handles all edge cases correctly, especially given its importance in Timeline relation processing.
web/core/components/gantt-chart/chart/views/quarter.tsx (1)
Line range hint
13-13: Consider improving type safety and styling maintainabilityThe component could benefit from the following improvements:
- Replace
FC<any>with proper TypeScript types for better type safety- Consider using CSS variables for the background colors to make theme changes more maintainable
Example implementation:
- export const QuarterChartView: FC<any> = observer(() => { + interface QuarterChartViewProps { + // Add relevant props here + } + + export const QuarterChartView: FC<QuarterChartViewProps> = observer(() => {And in your CSS:
:root { --highlight-bg-opacity: 0.2; --highlight-bg-color: var(--custom-primary-100); }web/core/components/gantt-chart/blocks/block-row.tsx (1)
102-102: Consider improving accessibility and error handling.While the scroll handler implementation is functional, consider these improvements:
- Add ARIA attributes to better describe the button's purpose
- Add error handling for undefined block scenarios
- Consider memoizing the handler to prevent unnecessary re-renders
Here's a suggested improvement:
- onClick={() => handleScrollToBlock(block)} + aria-label={`Scroll to ${block?.data?.title || 'block'}`} + onClick={() => block && handleScrollToBlock(block)}web/core/components/gantt-chart/chart/views/month.tsx (1)
95-95: Consider extracting the repeated background style into a constant.The background color
bg-custom-primary-100/20is used consistently for highlighting the current week in both the subtitle and column sections. To maintain this consistency and make future updates easier, consider extracting this into a constant.+const CURRENT_WEEK_HIGHLIGHT_CLASS = "bg-custom-primary-100/20"; + export const MonthChartView: FC<any> = observer(() => { // ... existing code ... // Then use it in both places: // Line 70: className={cn("flex flex-shrink-0 py-1 px-2 text-center capitalize justify-between outline-[0.25px] outline outline-custom-border-200", { - "bg-custom-primary-100/20": weekBlock.today, + [CURRENT_WEEK_HIGHLIGHT_CLASS]: weekBlock.today, })} // Line 95: className={cn("h-full overflow-hidden outline-[0.25px] outline outline-custom-border-100", { - "bg-custom-primary-100/20": weekBlock.today, + [CURRENT_WEEK_HIGHLIGHT_CLASS]: weekBlock.today, })}web/core/components/gantt-chart/views/quarter-view.ts (1)
76-85: Consider extracting date calculation logic.The date calculation logic is similar between left and right side handling. Consider extracting this into a helper function to:
- Reduce code duplication
- Centralize date validation
- Make the code more maintainable
+const calculateChartDates = ( + currentDate: Date, + referenceDate: Date, + range: number, + isLeftSide: boolean +): { minusDate: Date; plusDate: Date } => { + if (isLeftSide) { + return { + minusDate: new Date(currentDate.getFullYear(), currentDate.getMonth() - range / 2, 1), + plusDate: new Date(referenceDate.getFullYear(), referenceDate.getMonth() - 1, 1) + }; + } + return { + minusDate: new Date(referenceDate.getFullYear(), referenceDate.getMonth() + 1, 1), + plusDate: new Date(currentDate.getFullYear(), currentDate.getMonth() + range / 2, 1) + }; +}; else if (side === "left") { const chartStartDate = renderState.data.startDate; const currentDate = targetDate ? targetDate : chartStartDate; - minusDate = new Date(currentDate.getFullYear(), currentDate.getMonth() - range / 2, 1); - plusDate = new Date(chartStartDate.getFullYear(), chartStartDate.getMonth() - 1, 1); + const dates = calculateChartDates(currentDate, chartStartDate, range, true); + minusDate = dates.minusDate; + plusDate = dates.plusDate; } else if (side === "right") { const chartEndDate = renderState.data.endDate; const currentDate = targetDate ? targetDate : chartEndDate; - minusDate = new Date(chartEndDate.getFullYear(), chartEndDate.getMonth() + 1, 1); - plusDate = new Date(currentDate.getFullYear(), currentDate.getMonth() + range / 2, 1); + const dates = calculateChartDates(currentDate, chartEndDate, range, false); + minusDate = dates.minusDate; + plusDate = dates.plusDate;web/core/components/issues/issue-detail-widgets/relations/content.tsx (2)
66-66: Consider memoizing the relation options.While the hook usage is correct, consider memoizing the result to prevent unnecessary recalculations on re-renders.
Consider applying this change:
- const ISSUE_RELATION_OPTIONS = useTimeLineRelationOptions(); + const ISSUE_RELATION_OPTIONS = React.useMemo(() => useTimeLineRelationOptions(), []);
Line range hint
32-147: Consider adding error boundaries and cleanup.The component handles complex relation operations but could benefit from:
- An error boundary to gracefully handle relation operation failures
- Proper cleanup of relation subscriptions/listeners when the component unmounts
Consider wrapping the component with an error boundary and adding cleanup:
// Add at the component level useEffect(() => { return () => { // Cleanup any active relation operations setIssueCrudState({ update: { toggle: false, issueId: undefined, issue: undefined }, delete: { toggle: false, issueId: undefined, issue: undefined } }); }; }, []);web/core/components/gantt-chart/views/month-view.ts (2)
66-75: Consider refactoring date calculations for better maintainabilityWhile the logic is correct, consider extracting the date calculations into a helper function to improve readability and maintainability. Also, the variable naming could be more consistent.
Consider this refactor:
- const chartStartDate = renderState.data.startDate; - const currentDate = targetDate ? targetDate : chartStartDate; - - minusDate = new Date(currentDate.getFullYear(), currentDate.getMonth() - range, 1); - plusDate = new Date(chartStartDate.getFullYear(), chartStartDate.getMonth(), chartStartDate.getDate() - 1); + const referenceDate = renderState.data.startDate; + const baseDate = targetDate ?? referenceDate; + + const calculateChartDates = (date: Date, rangeMonths: number) => ({ + minus: new Date(date.getFullYear(), date.getMonth() - rangeMonths, 1), + plus: new Date(referenceDate.getFullYear(), referenceDate.getMonth(), referenceDate.getDate() - 1) + }); + + const { minus: minusDate, plus: plusDate } = calculateChartDates(baseDate, range);
83-94: Consider unifying left and right side chart generation logicThe left and right side chart generation logic is similar but mirrored. Consider creating a unified approach to handle both cases.
Here's a suggested approach:
+ type ChartSide = 'left' | 'right'; + + const generateSideChart = (side: ChartSide, referenceDate: Date, targetDate: Date | undefined) => { + const baseDate = targetDate ?? referenceDate; + const isLeft = side === 'left'; + + return { + minus: new Date( + isLeft ? baseDate.getFullYear() : referenceDate.getFullYear(), + isLeft ? baseDate.getMonth() - range : referenceDate.getMonth(), + isLeft ? 1 : referenceDate.getDate() + 1 + ), + plus: new Date( + isLeft ? referenceDate.getFullYear() : baseDate.getFullYear(), + isLeft ? referenceDate.getMonth() : baseDate.getMonth() + range, + isLeft ? referenceDate.getDate() - 1 : 1 + ) + }; + }; - const chartEndDate = renderState.data.endDate; - const currentDate = targetDate ? targetDate : chartEndDate; - - minusDate = new Date(chartEndDate.getFullYear(), chartEndDate.getMonth(), chartEndDate.getDate() + 1); - plusDate = new Date(currentDate.getFullYear(), currentDate.getMonth() + range, 1); + const { minus: minusDate, plus: plusDate } = generateSideChart( + 'right', + renderState.data.endDate, + targetDate + );This refactor would:
- Reduce code duplication
- Make the logic more maintainable
- Make it easier to add new side handling in the future if needed
web/core/components/issues/issue-detail/relation-select.tsx (1)
Line range hint
31-158: Consider extracting relation item to a separate component.The component implementation looks solid and meets all PR objectives. However, for better maintainability, consider extracting the relation item rendering logic (the mapped items JSX) into a separate component. This would:
- Improve code organization
- Make testing easier
- Allow for better reusability
Example structure:
// components/issues/issue-detail/relation-item.tsx type RelationItemProps = { relationIssueId: string; workspaceSlug: string; disabled: boolean; onRemove: () => void; className: string; }; const RelationItem: React.FC<RelationItemProps> = ({ ... }) => { // Extract the relation item JSX here };web/core/components/gantt-chart/views/week-view.ts (1)
Line range hint
41-99: Consider adding comprehensive date handling tests.While the implementation successfully addresses the timeline layout and scrolling requirements, the date handling logic would benefit from comprehensive testing. Consider adding test cases for:
- Month/year boundary transitions
- Leap years
- Different time zones
- Invalid date combinations
Consider extracting the date manipulation logic into a separate utility class with proper validation and error handling. This would:
- Centralize date handling logic
- Make it easier to test edge cases
- Improve maintainability
- Enable reuse across other views (day, month, etc.)
web/core/components/gantt-chart/chart/root.tsx (2)
112-115: Review setTimeout usage for scroll handling.The use of setTimeout for scroll handling could lead to race conditions or flickering. Consider using React's useEffect or requestAnimationFrame for more reliable DOM updates.
Consider this alternative approach:
- setTimeout(() => { - handleScrollToCurrentSelectedDate(currentRender.state, currentRender.state.data.currentDate); - }, 50); + useEffect(() => { + if (currentRender.state && currentRender.state.data.currentDate) { + requestAnimationFrame(() => + handleScrollToCurrentSelectedDate(currentRender.state, currentRender.state.data.currentDate) + ); + } + }, [currentRender.state]);
Line range hint
89-152: Overall implementation effectively addresses timeline layout issues.The changes successfully implement the scrolling functionality mentioned in the PR objectives. The code handles both normal navigation and targeted scrolling to specific blocks, with proper state management and view updates.
A few suggestions for future improvements:
- Consider using a ref for the gantt container instead of querySelector
- Add error boundaries for DOM operations
- Implement more robust scroll handling
web/core/components/gantt-chart/chart/main-content.tsx (1)
116-134: Consider improving scroll handling robustness.While the implementation works, there are several areas for improvement:
- Add error handling for invalid dates
- Clean up setTimeout on component unmount
- Document or extract the magic number (-4) as a constant
- Consider adding loading state during scroll operations
Here's a suggested improvement:
+ const SCROLL_OFFSET = -4; // Offset to align block with left edge + const SCROLL_DELAY = 0; // Delay for scroll animation + const handleScrollToBlock = (block: IGanttBlock) => { const scrollContainer = ganttContainerRef.current as HTMLDivElement; const scrollToDate = getDate(block.start_date); let chartData; - if (!scrollContainer || !currentViewData || !scrollToDate) return; + if (!scrollContainer || !currentViewData) return; + if (!scrollToDate) { + console.warn('Invalid date for block:', block.id); + return; + } if (scrollToDate.getTime() < currentViewData.data.startDate.getTime()) { chartData = updateCurrentViewRenderPayload("left", currentView, scrollToDate); } else if (scrollToDate.getTime() > currentViewData.data.endDate.getTime()) { chartData = updateCurrentViewRenderPayload("right", currentView, scrollToDate); } const updatedPosition = getItemPositionWidth(chartData ?? currentViewData, block); - setTimeout(() => { + const timeoutId = setTimeout(() => { - if (updatedPosition) scrollContainer.scrollLeft = updatedPosition.marginLeft - 4; + if (updatedPosition) scrollContainer.scrollLeft = updatedPosition.marginLeft + SCROLL_OFFSET; }); + + return () => clearTimeout(timeoutId); };web/core/store/issue/issue-details/relation.store.ts (1)
92-102: Consider adding input validation and early returnsThe implementation is solid with good use of
computedFnfor memoization and proper null handling. Consider these improvements:getRelationCountByIssueId = computedFn( (issueId: string, ISSUE_RELATION_OPTIONS: { [key in TIssueRelationTypes]?: TRelationObject }) => { + if (!issueId || !ISSUE_RELATION_OPTIONS) return 0; + const issueRelations = this.getRelationsByIssueId(issueId); + if (!issueRelations) return 0; const issueRelationKeys = (Object.keys(issueRelations ?? {}) as TIssueRelationTypes[]).filter( (relationKey) => !!ISSUE_RELATION_OPTIONS[relationKey] ); return issueRelationKeys.reduce((acc, curr) => acc + (issueRelations?.[curr]?.length ?? 0), 0); } );apiserver/plane/app/views/issue/relation.py (1)
Line range hint
336-347: Add error handling for relation removal.While the addition of timeline relation types is correct, the code could benefit from proper error handling when the relation doesn't exist.
Consider wrapping the relation retrieval in a try-except block:
- if relation_type in ["blocking", "start_after", "finish_after"]: - issue_relation = IssueRelation.objects.get( - workspace__slug=slug, - project_id=project_id, - issue_id=related_issue, - related_issue_id=issue_id, - ) - else: - issue_relation = IssueRelation.objects.get( - workspace__slug=slug, - project_id=project_id, - issue_id=issue_id, - related_issue_id=related_issue, - ) + try: + if relation_type in ["blocking", "start_after", "finish_after"]: + issue_relation = IssueRelation.objects.get( + workspace__slug=slug, + project_id=project_id, + issue_id=related_issue, + related_issue_id=issue_id, + ) + else: + issue_relation = IssueRelation.objects.get( + workspace__slug=slug, + project_id=project_id, + issue_id=issue_id, + related_issue_id=related_issue, + ) + except IssueRelation.DoesNotExist: + return Response( + {"error": "Issue relation not found"}, + status=status.HTTP_404_NOT_FOUND + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (22)
apiserver/plane/app/views/issue/relation.py(2 hunks)web/ce/components/relations/activity.ts(1 hunks)web/ce/components/relations/index.tsx(2 hunks)web/core/components/gantt-chart/blocks/block-row-list.tsx(2 hunks)web/core/components/gantt-chart/blocks/block-row.tsx(2 hunks)web/core/components/gantt-chart/chart/main-content.tsx(5 hunks)web/core/components/gantt-chart/chart/root.tsx(5 hunks)web/core/components/gantt-chart/chart/views/month.tsx(2 hunks)web/core/components/gantt-chart/chart/views/quarter.tsx(2 hunks)web/core/components/gantt-chart/chart/views/week.tsx(2 hunks)web/core/components/gantt-chart/views/month-view.ts(2 hunks)web/core/components/gantt-chart/views/quarter-view.ts(2 hunks)web/core/components/gantt-chart/views/week-view.ts(2 hunks)web/core/components/issues/issue-detail-widgets/issue-detail-widget-collapsibles.tsx(2 hunks)web/core/components/issues/issue-detail-widgets/relations/content.tsx(2 hunks)web/core/components/issues/issue-detail-widgets/relations/quick-action-button.tsx(3 hunks)web/core/components/issues/issue-detail-widgets/relations/title.tsx(2 hunks)web/core/components/issues/issue-detail/issue-activity/activity/actions/relation.tsx(2 hunks)web/core/components/issues/issue-detail/issue-activity/activity/activity-list.tsx(3 hunks)web/core/components/issues/issue-detail/relation-select.tsx(5 hunks)web/core/store/issue/issue-details/relation.store.ts(3 hunks)web/helpers/array.helper.ts(2 hunks)
🔇 Additional comments (43)
web/ce/components/relations/activity.ts (2)
1-2: LGTM: Clean and necessary import.
The type import is well-defined and provides proper type safety for the function parameter.
19-20: LGTM: Explicit return is good practice.
While TypeScript would infer undefined return, being explicit improves code clarity and maintainability.
web/core/components/issues/issue-detail/issue-activity/activity/actions/relation.tsx (2)
6-6: LGTM! Good move to dynamic relation options.
The switch from static ISSUE_RELATION_OPTIONS to dynamic options via hooks aligns well with the Timeline layout improvements and provides more flexibility in handling relations.
27-27: Verify type safety for relation field access.
The type casting of activity.field to TIssueRelationTypes could be unsafe. Consider adding runtime validation.
✅ Verification successful
Let me gather more information about how the activity.field is being used and validated.
Let me check one more thing to understand how the activity type is defined and validated.
Let me try one last search with a corrected pattern.
Based on the gathered information, I can now generate the final response:
Type casting to TIssueRelationTypes is safe in this context
The type casting is safe because:
TIssueRelationTypesis strictly defined as"blocking" | "blocked_by" | "duplicate" | "relates_to"- The
ISSUE_RELATION_OPTIONSobject is type-safe and only contains these specific relation types - The optional chaining (
?.) provides additional runtime safety if the field is undefined
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check all possible values of activity.field in the codebase
rg -A 2 "activity\.field.*relation"
Length of output: 241
Script:
#!/bin/bash
# Check ISSUE_RELATION_OPTIONS definition and usage
rg "ISSUE_RELATION_OPTIONS.*=" -A 5
# Check if there's any validation for activity.field
ast-grep --pattern 'activity.field'
# Check the type definition of the activity prop/parameter
ast-grep --pattern 'activity: $_'
Length of output: 8160
Script:
#!/bin/bash
# Check the type definitions related to activity
rg "type.*Activity.*{" -A 10
# Check if there's any validation before the activity reaches this component
ast-grep --pattern 'function getRelationActivityContent($_) {
$$$
}'
Length of output: 201
Script:
#!/bin/bash
# Check the type definitions related to activity
rg "interface.*Activity" -A 10
rg "type.*Activity" -A 10
# Check how activity.field is validated in the relation component
ast-grep --pattern 'const RelationActivity = ({ activity }: $_) => {
$$$
}'
Length of output: 47610
web/ce/components/relations/index.tsx (1)
6-6: Verify the activity module existence.
Let's ensure the activity module exists to prevent potential runtime errors.
✅ Verification successful
The activity module exists and is correctly referenced
The activity module exists at web/ce/components/relations/activity.ts and is correctly imported in index.tsx. The export statement is valid and will work as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the activity module
# Expected: The activity module should exist in the same directory
fd --type f "activity.{ts,tsx}" "$(dirname web/ce/components/relations/index.tsx)"
Length of output: 424
Script:
#!/bin/bash
# Let's try with glob pattern to find the activity module
fd -g "activity.{ts,tsx}" "web/ce/components/relations"
# Also search for any imports of activity to verify its existence
rg "from ['\"].*activity['\"]" "web/ce/components/relations"
Length of output: 226
web/core/components/issues/issue-detail-widgets/relations/title.tsx (2)
9-10: LGTM: Clean import addition
The new import for useTimeLineRelationOptions is properly structured and follows the project's import conventions.
25-25: Consider adding explicit type for relation options
While the code functions correctly, consider adding explicit typing for ISSUE_RELATION_OPTIONS to improve type safety and code maintainability.
Let's verify the consistency of the getRelationCountByIssueId function signature across the codebase:
Also applies to: 27-27
web/core/components/issues/issue-detail-widgets/relations/quick-action-button.tsx (1)
9-9: LGTM! Verify hook usage across Timeline components.
The switch to useTimeLineRelationOptions hook aligns with the dynamic approach mentioned in the PR objectives.
Let's verify this hook's usage across Timeline components:
✅ Verification successful
Hook implementation and usage are consistent
The verification shows that:
- The hook is correctly implemented in
web/ce/components/relations/index.tsxas a simple wrapper aroundISSUE_RELATION_OPTIONS - It's being used consistently in the quick-action-button component
- The hook is imported and used across multiple Timeline-related components as seen in the first search results
- No legacy static imports of
ISSUE_RELATION_OPTIONSwere found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the hook is consistently used across Timeline components
# and there are no remaining static imports of ISSUE_RELATION_OPTIONS
# Test 1: Find all Timeline-related components using this hook
rg -l "useTimeLineRelationOptions"
# Test 2: Check for any remaining static imports that might need updating
rg "import.*ISSUE_RELATION_OPTIONS.*from.*plane-web/components/relations"
Length of output: 680
Script:
#!/bin/bash
# Let's verify the hook implementation and its usage pattern
# Check the hook implementation
ast-grep --pattern 'export const useTimeLineRelationOptions = $_'
# Check how the hook is being used in components
rg -A 5 "useTimeLineRelationOptions" web/core/components/issues/issue-detail-widgets/relations/quick-action-button.tsx
rg -A 5 "useTimeLineRelationOptions" web/ce/components/relations/index.tsx
Length of output: 876
web/core/components/gantt-chart/blocks/block-row-list.tsx (3)
8-8: LGTM!
The IGanttBlock type import is correctly added and properly grouped with other type imports.
22-30: LGTM!
Props destructuring is clean and consistent with the existing pattern.
49-49: Verify scroll behavior integration.
Since this PR aims to fix scrolling to blocks beyond chart limits, please ensure that:
- The scroll behavior works correctly when blocks are outside the visible area
- The scroll position calculation takes into account the chart's boundaries
- The scrolling is smooth and doesn't cause layout shifts
✅ Verification successful
Let me try another search strategy to verify the scroll behavior integration.
Let me try one more search focusing specifically on the Gantt chart scroll functionality.
Scroll behavior integration is properly implemented
The implementation in main-content.tsx shows a robust scroll handling mechanism that:
- Handles blocks outside visible area by checking if the scroll target date is beyond current view boundaries
- Updates the chart view accordingly using
updateCurrentViewRenderPayload - Properly propagates the scroll handler through the component hierarchy (main-content → block-row-list → block-row)
- Includes visibility checks to prevent unnecessary scrolling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for scroll-related implementations and tests
echo "Searching for scroll-related implementations:"
rg -l "scroll" --type ts --type tsx
echo "\nSearching for specific scroll handler implementation:"
ast-grep --pattern 'handleScrollToBlock = (block: $_) => { $$$ }'
Length of output: 353
Script:
#!/bin/bash
# Search for scroll-related implementations with proper file extensions
echo "Searching for scroll-related implementations:"
rg -l "scroll" --type typescript --type tsx
echo "\nSearching for handleScrollToBlock implementation:"
rg -A 5 "handleScrollToBlock" --type typescript --type tsx
echo "\nSearching for scroll-related test files:"
fd -e test.ts -e test.tsx | xargs rg "scroll" -l
Length of output: 7986
Script:
#!/bin/bash
# Search for scroll-related code in Gantt chart components
echo "Searching for scroll handler in Gantt chart components:"
rg -A 10 "handleScrollToBlock" web/core/components/gantt-chart/
echo "\nSearching for scroll-related code in block components:"
rg -A 5 "scroll" web/core/components/gantt-chart/blocks/
Length of output: 8771
web/core/components/issues/issue-detail-widgets/issue-detail-widget-collapsibles.tsx (2)
13-14: LGTM: Clean import organization
The new import is well-organized with a clear section comment.
36-37: Consider performance optimization and verify consistency
While the implementation aligns with the PR's objective of fixing relation handling, consider these improvements:
- The hook result could be memoized if the options are static:
- const ISSUE_RELATION_OPTIONS = useTimeLineRelationOptions();
+ const ISSUE_RELATION_OPTIONS = React.useMemo(() => useTimeLineRelationOptions(), []);- The method signature change for
getRelationCountByIssueIdneeds consistent updates across the codebase.
web/core/components/issues/issue-detail/issue-activity/activity/activity-list.tsx (2)
3-4: LGTM: Clean import organization
The new imports are well-organized and properly categorized under helpers and hooks sections.
Also applies to: 9-9
44-45: Consider adding error handling for the timeline options hook
The useTimeLineRelationOptions hook result is used directly without any error handling. Consider adding error handling to gracefully handle cases where the hook fails or returns unexpected data.
- const ISSUE_RELATION_OPTIONS = useTimeLineRelationOptions();
- const activityRelations = getValidKeysFromObject(ISSUE_RELATION_OPTIONS);
+ const issueRelationOptions = useTimeLineRelationOptions();
+ const activityRelations = issueRelationOptions
+ ? getValidKeysFromObject(issueRelationOptions)
+ : [];web/helpers/array.helper.ts (1)
100-104: LGTM! Input validation and implementation are solid.
The function implementation:
- Properly validates input with comprehensive checks
- Correctly handles edge cases
- Uses efficient lodash isEmpty for object validation
- Returns expected results with a clean filter operation
web/core/components/gantt-chart/chart/views/week.tsx (3)
52-52: Improved dark mode compatibility for today's date highlight
The change to bg-custom-primary-100/20 is a good improvement as it provides better contrast and compatibility with dark mode through opacity-based styling.
74-74: Enhanced theme consistency for chart background
The addition of bg-custom-background-100 ensures consistent background styling that properly adapts to both light and dark modes.
Line range hint 79-85: Verify visual clarity when today falls on weekends
While the change to bg-custom-primary-100/20 improves dark mode support, please verify that the highlight remains visually distinct when today's column overlaps with weekend columns (which have bg-custom-background-90).
To help verify this, you can check how it looks by running this layout with different dates:
✅ Verification successful
Let's also check for any existing color-related styling or theme configurations:
And let's look for any existing visual tests or documentation:
Based on the search results, I can now provide a conclusive response about the visual clarity concern when today's column overlaps with weekend columns:
No visual conflict between weekend and today's highlight
The implementation ensures visual distinction through layered styling:
- Weekend columns use
bg-custom-background-90with an outline - Today's column uses
bg-custom-primary-100/20(20% opacity primary color) - The weekend background is rendered as a child div inside the column, while today's highlight is on the column itself
This layering approach means:
- The styles won't conflict as they're applied at different levels
- Both visual indicators will be visible when they overlap
- The opacity-based highlight for today's column will still be distinguishable over the weekend's background
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any test files or stories that might help visualize this component
fd -e test.tsx -e stories.tsx -e spec.tsx | grep -i "gantt\|timeline\|week"
Length of output: 77
Script:
#!/bin/bash
# Let's try a broader search to find any test or story files related to the Gantt chart component
fd "gantt|week|timeline" -e tsx -e ts
# Also search for any visual styling tests or stories in the web/core directory
fd -t f . web/core/components/gantt-chart -e tsx -e ts
Length of output: 3329
Script:
#!/bin/bash
# Search for color-related styling configurations
rg "bg-custom-(primary|background)" web/core/components/gantt-chart -A 2 -B 2
Length of output: 22011
Script:
#!/bin/bash
# Search for any documentation or test files
fd "(test|spec|stories|README)" web/core/components/gantt-chart
Length of output: 65
web/core/components/gantt-chart/chart/views/quarter.tsx (2)
83-83: Consistent styling applied to month columns
The same background styling is correctly applied to the month columns, maintaining visual consistency with the header section.
59-59: Improved visual consistency for dark mode
The change from bg-custom-primary-10 to bg-custom-primary-100/20 provides better contrast in both light and dark modes by using opacity instead of a fixed color shade.
Let's verify if this color class is consistently used across other timeline views:
✅ Verification successful
Background color class is consistently used across all timeline views
The verification confirms that the bg-custom-primary-100/20 class is uniformly applied across all timeline views (week, month, and quarter) for highlighting today's date blocks, maintaining visual consistency throughout the Gantt chart component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent usage of the background color class across timeline views
rg "bg-custom-primary-\d+/20" web/core/components/gantt-chart/
Length of output: 786
web/core/components/gantt-chart/blocks/block-row.tsx (3)
13-13: LGTM: Import addition is appropriate.
The addition of IGanttBlock type import aligns with the new prop type requirements.
19-19: LGTM: Prop type definition is well-structured.
The handleScrollToBlock prop type is properly defined and aligns with the PR's scrolling functionality objective.
26-26: LGTM: Props destructuring is correct.
The handleScrollToBlock prop is properly destructured alongside other related props.
web/core/components/gantt-chart/chart/views/month.tsx (1)
70-70: Background color change improves dark mode compatibility.
The switch to bg-custom-primary-100/20 provides better contrast control through explicit opacity, which is beneficial for dark mode support.
Let's verify the styling in both light and dark modes:
✅ Verification successful
Based on the search results, I can see that bg-custom-primary-100/20 is widely used throughout the codebase for various UI elements, but I don't see any dark mode specific overrides for this color. Let me verify if there are any dark mode color configurations in the theme system.
Let me check the tailwind configuration files to understand how the primary colors are defined and if there are any specific dark mode configurations.
Let me check how the primary colors are defined in the CSS variables, as the tailwind config shows they are using CSS variables for colors.
Based on the comprehensive analysis of the codebase, I can now provide a final response regarding the background color change:
Background color change is appropriate for dark mode compatibility
The change from bg-custom-primary-10 to bg-custom-primary-100/20 is a valid improvement because:
- The codebase uses a CSS variable-based theming system with proper dark mode support (
darkMode: "class"in Tailwind config) - The new color uses opacity (
/20) which works better with dark mode as it preserves contrast by being semi-transparent - The color values are defined consistently across the application using CSS variables, ensuring theme consistency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any theme-specific overrides or related styles
# that might affect this background color
# Look for related color definitions or theme variables
rg -g '*.{css,scss,ts,tsx}' 'custom-primary-100'
# Check for any dark mode specific overrides
rg -g '*.{css,scss,ts,tsx}' 'dark:bg-custom-primary'
Length of output: 37758
Script:
#!/bin/bash
# Check for theme configuration files and dark mode color definitions
rg -g '*.{js,ts,tsx,json}' -i "darkMode|dark.*theme|dark:.*primary"
# Look for any tailwind configuration files that might define these colors
fd -e js -e ts -e json tailwind.config
Length of output: 2631
Script:
#!/bin/bash
# Check the content of tailwind config files for color definitions
cat packages/tailwind-config-custom/tailwind.config.js
cat web/tailwind.config.js
Length of output: 17849
Script:
#!/bin/bash
# Look for CSS files that define the primary color variables
rg -g '*.{css,scss}' '\-\-color\-primary'
Length of output: 11011
web/core/components/gantt-chart/views/quarter-view.ts (2)
22-22: LGTM: Function signature change enhances flexibility.
The addition of the optional targetDate parameter allows for more precise control over chart generation, which aligns with the PR's objective of implementing scrolling to specific blocks.
58-68: Verify date handling edge cases.
While the logic for left-side chart generation looks correct, please ensure that:
chartStartDateis always defined when accessed- Date arithmetic produces valid results when
targetDateis at month boundaries
✅ Verification successful
Based on the search results, I can see that chartStartDate is accessed from renderState.data.startDate in the Gantt chart views. Let me verify the initialization of this data structure.
Let me try a different approach to find the renderState initialization.
Let me check the payload initialization to ensure startDate is properly defined.
Let me check the ChartDataType definition to understand how startDate is initialized.
Based on the code analysis, I can now provide a final response regarding the date handling verification:
Date handling in quarter view is properly implemented and safe.
The code is well-structured with proper safeguards:
chartStartDateis guaranteed to be defined as it comes fromrenderState.data.startDate, which is a required field in theChartDataTypeDatainterface- The date arithmetic is safe with proper boundary handling:
- Left-side date calculations use proper month arithmetic with range/2
- Right-side date calculations maintain proper date boundaries
- The
getMonthsBetweenTwoDatesis only called after validating bothminusDateandplusDateare defined
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all places where startDate is set or validated
rg -A 3 'startDate.*=' --type ts
Length of output: 19226
Script:
#!/bin/bash
# Search for renderState initialization and data structure
ast-grep --pattern 'renderState = {
$$$
data: {
$$$
startDate: $_,
$$$
},
$$$
}'
Length of output: 102
Script:
#!/bin/bash
# Search for renderState initialization in the gantt chart views
rg -A 5 'renderState\s*=' web/core/components/gantt-chart/views/
Length of output: 5802
Script:
#!/bin/bash
# Search for quarterPayload initialization and type definition
rg -B 5 -A 5 'quarterPayload|IQuarterPayload' web/core/components/gantt-chart/views/
Length of output: 2318
Script:
#!/bin/bash
# Search for ChartDataType definition and its usage
rg -B 3 -A 5 'ChartDataType|type.*Chart.*=' web/core/components/gantt-chart/
Length of output: 19567
web/core/components/issues/issue-detail-widgets/relations/content.tsx (1)
13-13: LGTM! Verify hook usage across components.
The switch to useTimeLineRelationOptions hook aligns with the PR's objective of fixing relation handling. This change provides more dynamic and context-aware relation options.
Let's verify this hook's usage across other components:
✅ Verification successful
Migration to useTimeLineRelationOptions hook is consistent across the codebase
The verification shows that:
- No legacy static imports of
ISSUE_RELATION_OPTIONSremain in the codebase - The new
useTimeLineRelationOptionshook is consistently used across all relevant components - The hook is properly defined in
web/ce/components/relations/index.tsx - All components using the hook follow the same pattern of initializing
ISSUE_RELATION_OPTIONS
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of the new hook across components
# and ensure no legacy ISSUE_RELATION_OPTIONS imports remain
# Search for any remaining static imports
echo "Checking for any remaining static ISSUE_RELATION_OPTIONS imports:"
rg "import.*ISSUE_RELATION_OPTIONS.*from.*@/plane-web/components/relations"
# Search for new hook usage
echo -e "\nChecking usage of new useTimeLineRelationOptions hook:"
rg "useTimeLineRelationOptions"
Length of output: 2571
web/core/components/gantt-chart/views/month-view.ts (2)
33-33: LGTM: Function signature enhancement
The addition of the optional targetDate parameter improves the function's flexibility in generating charts for specific dates, which aligns with the PR's objective of enhancing scrolling functionality.
Line range hint 33-94: Verify timeline layout integration
The changes look good but let's verify the integration with the timeline layout to ensure proper handling of all scenarios.
✅ Verification successful
Based on the gathered information, I can now generate the final response:
Timeline layout integration is properly maintained ✅
The changes to generateMonthChart function are well-integrated with the timeline layout system:
- The function is properly exported through the
monthViewobject and maintains its interface - Timeline drag functionality remains independent of the chart generation logic
- The changes only affect internal chart generation without impacting the broader timeline layout system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for timeline-related components that might be affected by these changes
# Look for timeline components that use this chart generation
echo "Searching for timeline components using month chart generation..."
rg -l "monthView.generateChart" --type ts --type tsx
# Look for tests related to timeline and month view
echo "Searching for related test files..."
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx | rg -i "(timeline|month.*view|gantt)"
Length of output: 392
Script:
#!/bin/bash
# Let's try a different approach to find related files and usages
# Search for files containing 'generateMonthChart' function calls
echo "Searching for generateMonthChart usages..."
rg "generateMonthChart" --type ts -l
# Search for month view related files
echo "Searching for month view related files..."
rg "MonthView" --type ts -l
# Look for timeline layout components
echo "Searching for timeline layout files..."
fd -e ts -e tsx | rg -i "timeline|gantt"
# Search for test files specifically
echo "Searching for test files..."
fd . -t f | rg "(test|spec)\.(ts|tsx)$"
Length of output: 4331
Script:
#!/bin/bash
# Let's examine the integration points more closely
# Check how the month view is integrated with the chart root
echo "Checking month view integration in chart root..."
rg -A 5 "month-view" web/core/components/gantt-chart/chart/root.tsx
# Look for the actual usage of generateMonthChart
echo "Checking generateMonthChart usage context..."
rg -B 2 -A 2 "generateMonthChart" web/core/components/gantt-chart/views/month-view.ts
# Check the timeline drag helper implementation
echo "Checking timeline drag implementation..."
cat web/core/components/gantt-chart/chart/timeline-drag-helper.tsx
# Check the types definition
echo "Checking related type definitions..."
rg "ChartDataType|IMonthView" web/core/components/gantt-chart/types/index.ts
Length of output: 1668
web/core/components/issues/issue-detail/relation-select.tsx (3)
18-21: LGTM! Well-organized imports.
The new imports for timeline relations are properly organized and align with the PR's objective of improving relation handling.
46-46: Verify hook memoization for performance.
The switch to dynamic relation options using useTimeLineRelationOptions is a good improvement. However, let's ensure the hook is properly memoized to prevent unnecessary re-renders.
Also applies to: 74-75
114-114: Verify dark mode compatibility.
The dynamic class names and placeholders from currRelationOption look good. This change aligns with the PR's goal of improving dark mode support.
Also applies to: 145-145
web/core/components/gantt-chart/views/week-view.ts (2)
41-41: LGTM: Function signature change maintains backward compatibility.
The addition of the optional targetDate parameter enhances the function's flexibility while maintaining backward compatibility. This change aligns well with the PR objective of implementing scrolling to specific blocks.
74-83: 🛠️ Refactor suggestion
Review date boundary handling in left-side calculations.
The current implementation has potential edge cases:
- Setting
minusDateto the first day of the month might skip weeks if the target date isn't at the start of a month - Month decrement operation doesn't handle year boundaries explicitly
Let's verify the date handling:
Consider this safer implementation:
- minusDate = new Date(currentDate.getFullYear(), currentDate.getMonth() - range, 1);
+ // Safely handle month/year boundaries
+ const targetMonth = new Date(currentDate.getTime());
+ targetMonth.setMonth(targetMonth.getMonth() - range);
+ minusDate = new Date(targetMonth.getFullYear(), targetMonth.getMonth(), currentDate.getDate());web/core/components/gantt-chart/chart/root.tsx (2)
Line range hint 89-99: LGTM: Function signature update enhances chart navigation.
The addition of the optional targetDate parameter aligns well with the PR objective of enabling scrolling to specific blocks. The implementation maintains backward compatibility with existing calls.
141-145: 🛠️ Refactor suggestion
Consider adding null checks and error handling for DOM operations.
The DOM query for #gantt-container could return null if the component hasn't mounted yet. Consider adding error boundaries or defensive programming.
Let's verify the container's existence across the codebase:
Consider this safer implementation:
const updateItemsContainerWidth = (width: number) => {
const scrollContainer = document.querySelector("#gantt-container") as HTMLDivElement;
if (!scrollContainer) return;
- setItemsContainerWidth(width + scrollContainer?.scrollLeft);
+ setItemsContainerWidth(width + (scrollContainer.scrollLeft || 0));
};web/core/components/gantt-chart/chart/main-content.tsx (3)
8-13: LGTM: Import additions are appropriate.
The new imports support the added scroll-to-block functionality and type safety improvements.
Also applies to: 21-21
53-57: LGTM: Props type changes enhance scroll functionality.
The updated signature with optional targetDate and explicit return type improves type safety while supporting the new scroll-to-block feature.
197-197: LGTM: Clean integration of scroll functionality.
The handleScrollToBlock prop is correctly passed to GanttChartRowList, maintaining good component composition.
web/core/store/issue/issue-details/relation.store.ts (2)
8-9: LGTM: Clean import addition
The import of TRelationObject is properly placed and necessary for the updated method signature.
43-46: Verify all callers of getRelationCountByIssueId
The method signature change is well-typed but introduces a breaking change. Let's verify all callers have been updated.
✅ Verification successful
All callers have been properly updated with the new parameter
The verification shows that all callers of getRelationCountByIssueId are already passing the required ISSUE_RELATION_OPTIONS parameter:
web/core/components/issues/issue-detail-widgets/issue-detail-widget-collapsibles.tsxweb/core/components/issues/issue-detail-widgets/relations/title.tsx
Both implementations are using the method with the correct signature, making this a safe change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of getRelationCountByIssueId to verify they've been updated with the new parameter
# Search for method calls
echo "Searching for getRelationCountByIssueId usage patterns..."
rg "getRelationCountByIssueId\(" --type ts --type tsx -A 2
Length of output: 213
Script:
#!/bin/bash
# Let's try a different approach using ripgrep without file type filters
echo "Searching for getRelationCountByIssueId usage patterns..."
rg "getRelationCountByIssueId\(" -A 2
# Also search for any file containing this method name to ensure we don't miss anything
echo -e "\nSearching for files containing the method name..."
rg -l "getRelationCountByIssueId"
# Let's also check the interface definition location
echo -e "\nSearching for interface or type definitions..."
ast-grep --pattern 'interface $_ {
$$$
getRelationCountByIssueId
$$$
}'
Length of output: 1393
apiserver/plane/app/views/issue/relation.py (1)
Line range hint 321-329: LGTM! Verify timeline relation serialization.
The addition of "start_after" and "finish_after" to the response serializer condition correctly handles timeline-specific relations. This aligns with the PR objective of fixing relation creation bugs.
Let's verify the serializer implementations:
✅ Verification successful
LGTM! The serializer implementation is correct for timeline relations
Both serializers (RelatedIssueSerializer and IssueRelationSerializer) include all necessary fields for timeline relations:
- They both expose
relation_typewhich handles the timeline-specific types ("start_after", "finish_after") - The model supports these relation types in its
RELATION_CHOICES - Both serializers include issue details (id, name, project_id) needed for timeline visualization
- The different serializer choice in the response is intentional as they provide different perspectives of the relation (issue vs. related_issue as the source)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that RelatedIssueSerializer and IssueRelationSerializer handle timeline relations correctly
# Expected: RelatedIssueSerializer should have fields necessary for timeline relations
# Search for serializer definitions
rg -A 10 "class (Related|Issue)RelationSerializer"
Length of output: 1751
Script:
#!/bin/bash
# Let's also check for RelatedIssueSerializer which was not found in the previous search
rg -A 10 "class RelatedIssueSerializer"
# Let's also check the IssueRelation model to understand the available fields
rg -A 10 "class IssueRelation"
Length of output: 6959
| switch (activity.field) { | ||
| case "blocking": | ||
| return activity.old_value === "" ? `marked this issue is blocking issue ` : `removed the blocking issue `; | ||
| case "blocked_by": | ||
| return activity.old_value === "" | ||
| ? `marked this issue is being blocked by ` | ||
| : `removed this issue being blocked by issue `; | ||
| case "duplicate": | ||
| return activity.old_value === "" ? `marked this issue as duplicate of ` : `removed this issue as a duplicate of `; | ||
| case "relates_to": | ||
| activity.old_value === "" ? `marked that this issue relates to ` : `removed the relation from `; | ||
| } |
There was a problem hiding this comment.
Fix critical bug in relates_to case and improve message consistency.
There are several issues in the switch statement:
- The
relates_tocase is missing areturnstatement, making it always returnundefined - Message formatting could be more consistent across cases
Apply these fixes:
switch (activity.field) {
case "blocking":
- return activity.old_value === "" ? `marked this issue is blocking issue ` : `removed the blocking issue `;
+ return activity.old_value === "" ? `marked this issue as blocking ` : `removed blocking relation from `;
case "blocked_by":
return activity.old_value === ""
- ? `marked this issue is being blocked by `
- : `removed this issue being blocked by issue `;
+ ? `marked this issue as blocked by `
+ : `removed blocked by relation from `;
case "duplicate":
return activity.old_value === "" ? `marked this issue as duplicate of ` : `removed this issue as a duplicate of `;
case "relates_to":
- activity.old_value === "" ? `marked that this issue relates to ` : `removed the relation from `;
+ return activity.old_value === "" ? `marked this issue as related to ` : `removed relation from `;
}📝 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.
| switch (activity.field) { | |
| case "blocking": | |
| return activity.old_value === "" ? `marked this issue is blocking issue ` : `removed the blocking issue `; | |
| case "blocked_by": | |
| return activity.old_value === "" | |
| ? `marked this issue is being blocked by ` | |
| : `removed this issue being blocked by issue `; | |
| case "duplicate": | |
| return activity.old_value === "" ? `marked this issue as duplicate of ` : `removed this issue as a duplicate of `; | |
| case "relates_to": | |
| activity.old_value === "" ? `marked that this issue relates to ` : `removed the relation from `; | |
| } | |
| switch (activity.field) { | |
| case "blocking": | |
| return activity.old_value === "" ? `marked this issue as blocking ` : `removed blocking relation from `; | |
| case "blocked_by": | |
| return activity.old_value === "" | |
| ? `marked this issue as blocked by ` | |
| : `removed blocked by relation from `; | |
| case "duplicate": | |
| return activity.old_value === "" ? `marked this issue as duplicate of ` : `removed this issue as a duplicate of `; | |
| case "relates_to": | |
| return activity.old_value === "" ? `marked this issue as related to ` : `removed relation from `; | |
| } |
| <span className="font-medium text-custom-text-100">{activity.old_value}.</span> | ||
| )} | ||
| </> | ||
| {activityContent} |
There was a problem hiding this comment.
💡 Codebase verification
Found issue with relation type handling
There's a bug in the getRelationActivityContent implementation where the relates_to case is missing a return statement, which could lead to undefined content. Here's the problematic code:
case "relates_to":
activity.old_value === "" ? `marked that this issue relates to ` : `removed the relation from `; // missing returnAll relation types from TIssueRelationTypes ("blocking", "blocked_by", "duplicate", "relates_to") are covered in the switch statement, but the relates_to case needs to be fixed.
🔗 Analysis chain
Verify handling of all relation types.
The simplified content rendering looks good, but let's ensure getRelationActivityContent handles all possible relation types correctly.
Let me gather more information about the relation content handling.
Let me check the complete implementation to verify all relation types are handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find the implementation of getRelationActivityContent
ast-grep --pattern 'function getRelationActivityContent($_) {
$$$
}'
# Find all relation types used in the codebase
rg "TIssueRelationTypes.*=.*['\"](\w+)['\"]"
Length of output: 241
Script:
#!/bin/bash
# Find the implementation of getRelationActivityContent and relation handling
rg -A 10 "getRelationActivityContent"
# Find any switch/case or if-else handling of relation types
ast-grep --pattern 'switch ($relation) {
$$$
}'
# Also check for if-else patterns
ast-grep --pattern 'if ($relation === $_) {
$$$
}'
Length of output: 3763
Script:
#!/bin/bash
# Get the complete implementation of getRelationActivityContent
rg -A 20 "export const getRelationActivityContent" web/ce/components/relations/activity.ts
# Also find any relation type constants or enums
rg "ISSUE_RELATION_OPTIONS" -A 5
Length of output: 11528
| {Object.values(ISSUE_RELATION_OPTIONS).map((item, index) => { | ||
| if (!item) return <></>; | ||
|
|
||
| return ( | ||
| <CustomMenu.MenuItem | ||
| key={index} | ||
| onClick={(e) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| handleOnClick(item.key as TIssueRelationTypes); | ||
| }} | ||
| > | ||
| <div className="flex items-center gap-2"> | ||
| {item.icon(12)} | ||
| <span>{item.label}</span> | ||
| </div> | ||
| </CustomMenu.MenuItem> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving menu item rendering robustness.
While the null check is a good addition, there are a few areas that could be improved:
- Using array index as key could cause issues with React's reconciliation if items are reordered
- The icon rendering assumes the icon function exists
- {Object.values(ISSUE_RELATION_OPTIONS).map((item, index) => {
+ {Object.values(ISSUE_RELATION_OPTIONS).map((item) => {
if (!item) return <></>;
return (
<CustomMenu.MenuItem
- key={index}
+ key={item.key}
onClick={(e) => {
e.preventDefault();
e.stopPropagation();
handleOnClick(item.key as TIssueRelationTypes);
}}
>
<div className="flex items-center gap-2">
- {item.icon(12)}
+ {item.icon ? item.icon(12) : null}
<span>{item.label}</span>
</div>
</CustomMenu.MenuItem>
);
})}📝 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.
| {Object.values(ISSUE_RELATION_OPTIONS).map((item, index) => { | |
| if (!item) return <></>; | |
| return ( | |
| <CustomMenu.MenuItem | |
| key={index} | |
| onClick={(e) => { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| handleOnClick(item.key as TIssueRelationTypes); | |
| }} | |
| > | |
| <div className="flex items-center gap-2"> | |
| {item.icon(12)} | |
| <span>{item.label}</span> | |
| </div> | |
| </CustomMenu.MenuItem> | |
| ); | |
| })} | |
| {Object.values(ISSUE_RELATION_OPTIONS).map((item) => { | |
| if (!item) return <></>; | |
| return ( | |
| <CustomMenu.MenuItem | |
| key={item.key} | |
| onClick={(e) => { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| handleOnClick(item.key as TIssueRelationTypes); | |
| }} | |
| > | |
| <div className="flex items-center gap-2"> | |
| {item.icon ? item.icon(12) : null} | |
| <span>{item.label}</span> | |
| </div> | |
| </CustomMenu.MenuItem> | |
| ); | |
| })} |
| case activityRelations.find((field) => field === activityField): | ||
| return <IssueRelationActivity {...componentDefaultProps} />; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize relation field check
The current implementation using array.find() is less efficient and harder to read. Using array.includes() would be more appropriate for this use case as we're only checking for membership.
- case activityRelations.find((field) => field === activityField):
+ case activityRelations.includes(activityField) ? activityField : undefined:This change:
- Improves performance by avoiding unnecessary iteration
- Makes the intent clearer - we're checking if the field is a valid relation
- Maintains the same behavior when no match is found
📝 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.
| case activityRelations.find((field) => field === activityField): | |
| return <IssueRelationActivity {...componentDefaultProps} />; | |
| case activityRelations.includes(activityField) ? activityField : undefined: | |
| return <IssueRelationActivity {...componentDefaultProps} />; |
| const chartEndDate = renderState.data.endDate; | ||
| const currentDate = targetDate ? targetDate : chartEndDate; | ||
|
|
||
| minusDate = new Date(currentDate.getFullYear(), currentDate.getMonth(), currentDate.getDate() + 1); | ||
| plusDate = new Date(currentDate.getFullYear(), currentDate.getMonth() + range, currentDate.getDate()); | ||
| minusDate = new Date(chartEndDate.getFullYear(), chartEndDate.getMonth(), chartEndDate.getDate() + 1); | ||
| plusDate = new Date(currentDate.getFullYear(), currentDate.getMonth() + range, 1); | ||
|
|
||
| if (minusDate && plusDate) filteredDates = getWeeksBetweenTwoDates(minusDate, plusDate); | ||
|
|
||
| startDate = new Date(currentDate.getFullYear(), currentDate.getMonth(), currentDate.getDate() + 1); | ||
| startDate = new Date(chartEndDate.getFullYear(), chartEndDate.getMonth(), chartEndDate.getDate() + 1); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review date boundary handling in right-side calculations.
Similar to the left-side calculations, there are potential edge cases:
- Setting
plusDateto the first day of the month might create inconsistent week ranges - Month increment operation doesn't handle year boundaries explicitly
Consider this safer implementation:
- plusDate = new Date(currentDate.getFullYear(), currentDate.getMonth() + range, 1);
+ // Safely handle month/year boundaries
+ const targetMonth = new Date(currentDate.getTime());
+ targetMonth.setMonth(targetMonth.getMonth() + range);
+ plusDate = new Date(targetMonth.getFullYear(), targetMonth.getMonth(), currentDate.getDate());Additionally, consider adding validation to ensure startDate is always before endDate:
if (startDate >= endDate) {
throw new Error('Invalid date range: startDate must be before endDate');
}
3 participants
This PR fixes few bugs in Timeline layout. The changes include,
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Chores