[WEB-2442] feat: Revamp Timeline Layout#5915
Conversation
…-listing-relations
…-listing-relations
…-listing-relations
…-listing-relations
…-listing-relations
…-listing-relations
…-listing-relations
…plane into chore/issue-listing-relations
WalkthroughThis pull request introduces several enhancements and modifications across various components of the application. Key changes include the addition of new serializers and endpoints for issue management, updates to the handling of issue relations, and the introduction of new components for Gantt chart functionality. Additionally, several existing components and files have been removed or refactored to streamline the codebase, particularly in the Gantt chart section, which now utilizes a more structured approach to data management and rendering. Changes
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 (
|
web/core/components/modules/gantt-chart/modules-list-layout.tsx
Outdated
Show resolved
Hide resolved
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: 57
🧹 Outside diff range and nitpick comments (56)
web/core/components/gantt-chart/constants.ts (1)
5-5: LGTM: New breadcrumbs height constant enhances navigation.The addition of
GANTT_BREADCRUMBS_HEIGHTsupports the PR's objective of revamping the Timeline layout by providing dedicated space for navigation between Week, Month, and Quarter views.Consider implementing responsive breakpoints if the combined height of header (48px) and breadcrumbs (40px) becomes too large on smaller screens.
web/core/components/gantt-chart/contexts/index.tsx (1)
1-13: Consider documenting the timeline architecture.As this is part of a larger Timeline revamp, it would be beneficial to:
- Add JSDoc comments explaining how this context fits into the new Week/Month/Quarter view architecture
- Consider creating a README.md in the gantt-chart directory documenting the overall architecture and data flow
Would you like me to help generate the documentation structure?
web/core/components/gantt-chart/chart/timeline-drag-helper.tsx (1)
1-7: Consider organizing imports following best practices.The imports could be better organized by grouping them into:
- External dependencies (react, mobx)
- Internal hooks
- Constants
import { RefObject } from "react"; import { observer } from "mobx-react"; -// hooks + import { useAutoScroller } from "@/hooks/use-auto-scroller"; import { useTimeLineChartStore } from "@/hooks/use-timeline-chart"; -// + import { HEADER_HEIGHT, SIDEBAR_WIDTH } from "../constants";apiserver/plane/utils/issue_relation_mapper.py (1)
1-10: Add docstring with type hints for better code documentation.The function would benefit from a clear docstring explaining its purpose, parameters, and return value.
Consider adding:
def get_inverse_relation(relation_type): + """ + Returns the inverse relation type for a given relation. + + Args: + relation_type (str): The relation type to inverse + + Returns: + str: The inverse relation type, or the original if no inverse exists + """web/core/components/gantt-chart/types/index.ts (1)
24-28: Add JSDoc comments to document the new interface.The new
IBlockUpdateDependencyDatainterface would benefit from documentation explaining its purpose and usage in dependency management.+/** + * Interface for updating block dependencies in the Gantt chart + * @property id - The unique identifier of the block + * @property start_date - Optional new start date for the dependency + * @property target_date - Optional new target date for the dependency + */ export interface IBlockUpdateDependencyData { id: string; start_date?: string; target_date?: string; }web/core/components/gantt-chart/blocks/blocks-list.tsx (1)
1-14: LGTM! Good architectural improvements.The removal of props like
getBlockByIdandblockUpdateHandlerin favor of a single asyncupdateBlockDatesfunction shows good architectural evolution. This change:
- Simplifies the component's API
- Provides better control over block updates through Promise-based operations
- Suggests a move towards more centralized state management
Consider documenting the expected format and validation requirements for the
IBlockUpdateDependencyDatatype to help other developers integrate with this component.web/core/components/gantt-chart/sidebar/modules/block.tsx (2)
25-26: Consider improving visibility check readability.The visibility check could be more explicit about its purpose.
- const isBlockVisibleOnChart = !!block.start_date && !!block.target_date; + const hasValidDateRange = !!block.start_date && !!block.target_date; + const isBlockVisibleOnChart = hasValidDateRange;
Line range hint
28-57: Consider extracting styles and duration text for better maintainability.The component could benefit from:
- Moving styles to constants
- Extracting the duration text to a reusable component
Example refactor:
// constants.ts export const SIDEBAR_BLOCK_STYLES = { wrapper: "group w-full flex items-center gap-2 pr-4", active: "bg-custom-background-90", dragging: "rounded bg-custom-background-80" }; // DurationText.tsx interface DurationTextProps { days: number; } export const DurationText: React.FC<DurationTextProps> = ({ days }) => ( <div className="flex-shrink-0 text-sm text-custom-text-200"> {days} day{days > 1 ? "s" : ""} </div> );web/core/components/gantt-chart/blocks/block-row-list.tsx (2)
11-18: Consider improving type safety forblockUpdateHandler.The
blockUpdateHandlerprop usesanytype for the block parameter, which bypasses TypeScript's type checking. Consider defining a specific interface for the block structure to improve type safety and maintainability.Example improvement:
interface IGanttBlock { id: string; // add other block properties } export type GanttChartBlocksProps = { // ... other props blockUpdateHandler: (block: IGanttBlock, payload: IBlockUpdateData) => void; // ... remaining props };
28-30: Consider extracting magic numbers to constants.The horizontal and vertical offset values (100 and 200) should be defined as named constants for better maintainability and reusability.
Example improvement:
const HORIZONTAL_SCROLL_OFFSET = 100; const VERTICAL_SCROLL_OFFSET = 200; // Then use in component: horizontalOffset={HORIZONTAL_SCROLL_OFFSET} verticalOffset={VERTICAL_SCROLL_OFFSET}web/core/components/gantt-chart/helpers/blockResizables/right-resizable.tsx (1)
34-38: Consider extracting z-index values to constants.The component uses hardcoded z-index values (
z-[10],z-[6],z-[5]). These should be extracted to constants or a configuration file to maintain consistency across the application.Create a constants file for z-index values:
// constants/z-index.ts export const ZINDEX = { TOOLTIP: 10, RESIZE_HANDLE: 6, RESIZE_INDICATOR: 5, } as const;Then update the component:
- className="z-[10] absolute flex..." + className={`z-[${ZINDEX.TOOLTIP}] absolute flex...`}Also applies to: 49-56
web/core/components/gantt-chart/chart/header.tsx (1)
32-35: Consider adding type safety for the height value.The Row component's height styling looks good, using a constant for consistency. However, we could enhance type safety.
- style={{ height: `${GANTT_BREADCRUMBS_HEIGHT}px` }} + style={{ height: GANTT_BREADCRUMBS_HEIGHT ? `${GANTT_BREADCRUMBS_HEIGHT}px` : undefined }}packages/types/src/issues/issue.d.ts (2)
5-5: Consider relocatingTIssueRelationTypesto avoid cross-package dependencies.The import path suggests that
TIssueRelationTypesis defined in the plane-web package. Since this is a core type definition used for issue relations, it would be more appropriate to define it within the types package to maintain a clear separation of concerns and prevent potential circular dependencies.Consider moving
TIssueRelationTypesfromplane-webto this package, possibly in a new file likeissue-relation-types.ts.
44-50: Add JSDoc comments to document the IssueRelation type.While the type definition is well-structured, adding documentation would improve maintainability and help other developers understand the purpose of each field, especially
relation_typeandsequence_id.Consider adding documentation like this:
+/** + * Represents a relationship between two issues + */ export type IssueRelation = { + /** Unique identifier for the relation */ id: string; + /** Name of the related issue */ name: string; + /** Project identifier containing the related issue */ project_id: string; + /** Type of relationship between issues */ relation_type: TIssueRelationTypes; + /** Order of the relation in the list */ sequence_id: number; };web/core/components/gantt-chart/root.tsx (1)
Line range hint
1-28: Well-structured integration of MobX for state management!The addition of MobX observer pattern and the well-typed props interface provides a solid foundation for reactive state management. This aligns well with the Timeline revamp objectives.
Consider documenting the feature toggle props (enable*) in JSDoc to help other developers understand when to use each flag.
web/core/components/core/render-if-visible-HOC.tsx (1)
Line range hint
69-75: Optimize height recording when forceRender is trueThe component still records heights even when
forceRenderis true, which may be unnecessary overhead.Consider skipping height recording when forcing render:
useEffect(() => { - if (intersectionRef.current && isVisible && shouldRecordHeights) { + if (intersectionRef.current && isVisible && shouldRecordHeights && !forceRender) { window.requestIdleCallback(() => { if (intersectionRef.current) placeholderHeight.current = `${intersectionRef.current.offsetHeight}px`; }); } - }, [isVisible, intersectionRef, shouldRecordHeights]); + }, [isVisible, intersectionRef, shouldRecordHeights, forceRender]);web/core/components/gantt-chart/helpers/add-block.tsx (1)
Line range hint
85-97: Enhance accessibility for the add button.Consider improving accessibility for keyboard and screen reader users:
<button type="button" className="absolute top-1/2 -translate-x-1/2 -translate-y-1/2 h-8 w-8 bg-custom-background-80 p-1.5 rounded border border-custom-border-300 grid place-items-center text-custom-text-200 hover:text-custom-text-100" style={{ marginLeft: `${buttonXPosition}px`, }} onClick={handleButtonClick} + aria-label={`Add block starting from ${buttonStartDate ? renderFormattedDate(buttonStartDate) : 'selected date'}`} + onKeyDown={(e) => { + if (e.key === 'ArrowLeft') setButtonXPosition(prev => Math.max(0, prev - currentViewData.data.dayWidth)); + if (e.key === 'ArrowRight') setButtonXPosition(prev => prev + currentViewData.data.dayWidth); + }} >web/core/components/gantt-chart/sidebar/issues/block.tsx (1)
30-31: Consider adding fallback for undefined position width.While the visibility check is good, there's no explicit handling for cases where
block?.position?.widthmight be undefined. Consider adding a fallback value or additional validation.Suggested improvement:
- const duration = isBlockVisibleOnChart ? getNumberOfDaysFromPosition(block?.position?.width) : undefined; + const duration = isBlockVisibleOnChart && block?.position?.width !== undefined + ? getNumberOfDaysFromPosition(block?.position?.width) + : undefined;web/core/components/gantt-chart/data/index.ts (3)
6-12: Standardize day abbreviations according to conventions.The abbreviation "Th" for Thursday should be changed to "Thu" to follow the common three-letter abbreviation convention used in most date libraries.
- { key: 4, shortTitle: "thurs", title: "thursday", abbreviation: "Th" }, + { key: 4, shortTitle: "thurs", title: "thursday", abbreviation: "Thu" },
16-28: Standardize month abbreviations to three letters.The abbreviation "Sept" for September should be "Sep" to maintain consistency with the three-letter abbreviation pattern used for other months.
- { key: 8, shortTitle: "sept", title: "september", abbreviation: "Sept" }, + { key: 8, shortTitle: "sept", title: "september", abbreviation: "Sep" },
64-94: Enhance documentation for view configurations.The view configurations would benefit from clearer documentation:
- Standardize comment style (all should end with periods).
- Add explanations for the significant differences in
dayWidthvalues.- Clarify the
approxFilterRangeimpact on view behavior.{ key: "week", title: "Week", data: { startDate: new Date(), currentDate: new Date(), endDate: new Date(), - approxFilterRange: 4, // it will preview week dates with weekends highlighted with 1 week limitations ex: title (Wed 1, Thu 2, Fri 3) + approxFilterRange: 4, // Controls the week view range. Shows 4 weeks with weekends highlighted. Example: "Wed 1, Thu 2, Fri 3". dayWidth: 60, // Wider width for detailed daily view }, }, { key: "month", title: "Month", data: { startDate: new Date(), currentDate: new Date(), endDate: new Date(), - approxFilterRange: 6, // it will preview monthly all dates with weekends highlighted with no limitations ex: title (1, 2, 3) + approxFilterRange: 6, // Controls the month view range. Shows 6 months with all dates and highlighted weekends. Example: "1, 2, 3". dayWidth: 20, // Medium width for monthly overview }, }, { key: "quarter", title: "Quarter", data: { startDate: new Date(), currentDate: new Date(), endDate: new Date(), - approxFilterRange: 18, // 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: 18, // Controls the quarter view range. Shows 18 months with week starting dates. Example: "2, 9, 16, 23, 30". dayWidth: 5, // Compact width for quarterly overview }, },packages/types/src/dashboard.d.ts (1)
Line range hint
1-150: Consider enhancing types for Timeline viewsGiven that this PR introduces Week, Month, and Quarter views for Timeline, consider:
- Adding specific widget types for Timeline views
- Including view-specific configuration types
- Adding types for Timeline-specific filters and responses
This would improve type safety and developer experience when working with the new Timeline features.
Example additions to consider:
export type TTimelineViewType = "week" | "month" | "quarter"; export type TTimelineWidgetFilters = { view_type: TTimelineViewType; custom_dates?: string[]; duration?: EDurationFilters; }; export type TTimelineWidgetResponse = { // Add specific response structure for Timeline widgets };web/core/components/gantt-chart/chart/views/month.tsx (3)
3-11: LGTM! Consider adding type safety to the component props.The hook and type changes align well with the new Timeline layout implementation. However, the component is using
FC<any>which bypasses TypeScript's type checking.Consider defining a proper interface for the component props:
interface MonthChartViewProps { // Add any props if needed, or leave empty if none required } export const MonthChartView: FC<MonthChartViewProps> = observer(() => {
28-61: Consider adding ARIA labels for better accessibility.The month header implementation looks good, but could benefit from improved accessibility.
Consider adding ARIA labels to improve screen reader experience:
<div key={`month-${monthBlock?.month}-${monthBlock?.year}`} + aria-label={`Month: ${monthBlock?.title}`} className="flex outline-[0.5px] outline outline-custom-border-200" style={{ width: `${monthBlock.days * currentViewData?.data.dayWidth}px` }} >
62-88: Consider memoizing week blocks for better performance.The week header implementation looks good, but could benefit from memoization to prevent unnecessary re-renders.
Consider using
useMemofor the week blocks:const weekBlocks = useMemo( () => weeks?.map((weekBlock) => ( <div key={`sub-title-${weekBlock.startDate}-${weekBlock.endDate}`} 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-10": weekBlock.today, } )} style={{ width: `${currentViewData?.data.dayWidth * 7}px` }} > {/* ... existing content ... */} </div> )), [weeks, currentViewData?.data.dayWidth] );apiserver/plane/app/views/__init__.py (1)
133-134: Consider adding type hints to improve code maintainability.Since these endpoints are crucial for the Timeline Layout feature, consider adding type hints to improve code maintainability and IDE support.
- IssueDetailEndpoint, - IssueBulkUpdateDateEndpoint, + IssueDetailEndpoint, # type: Type[ViewSet] + IssueBulkUpdateDateEndpoint, # type: Type[ViewSet]🧰 Tools
🪛 Ruff
133-133:
.issue.base.IssueDetailEndpointimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
134-134:
.issue.base.IssueBulkUpdateDateEndpointimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
web/core/components/issues/issue-detail-widgets/relations/content.tsx (3)
Line range hint
23-45: Consider enhancing type safety for relation options.The export of
ISSUE_RELATION_OPTIONSis a good move for reusability. However, we could improve type safety.Consider adding a type definition for the relation options:
type IssueRelationType = { key: 'blocked_by' | 'blocking' | 'relates_to' | 'duplicate'; label: string; icon: (size: number) => JSX.Element; className: string; }; export const ISSUE_RELATION_OPTIONS: IssueRelationType[] = [ // ... existing options ];
Line range hint
89-91: Consider enhancing error handling.While the null check for relations is good, consider adding more comprehensive error handling for edge cases.
Consider implementing:
- Error boundaries to catch rendering errors
- Loading states for async operations
- Fallback UI for failed relation fetches
// Example error boundary implementation class RelationsErrorBoundary extends React.Component { static getDerivedStateFromError(error: Error) { return { hasError: true }; } render() { if (this.state.hasError) { return <div className="text-red-500">Failed to load relations</div>; } return this.props.children; } }
Line range hint
47-63: Consider simplifying modal state management.The current modal state management works but could be simplified for better maintainability.
Consider using a discriminated union for modal state:
type ModalState = | { type: 'none' } | { type: 'update'; issue: TIssue } | { type: 'delete'; issue: TIssue }; const [modalState, setModalState] = useState<ModalState>({ type: 'none' });This would simplify your conditional rendering and state updates while maintaining type safety.
web/core/components/api-token/modal/form.tsx (2)
Line range hint
53-65: Remove unnecessaryundefinedfrom return type.The function's implementation only returns either a
string(when a valid date option is found) ornull(when no option is found). Theundefinedin the return type signature is unnecessary and could mislead consumers of this function.Apply this change:
-const getExpiryDate = (val: string): string | null | undefined => { +const getExpiryDate = (val: string): string | null => {
Line range hint
190-195: Add null check before displaying expiry date.The expiry date text is displayed without checking if
getExpiryDatereturns null, which could result in "Expires null" being shown to users.Apply this change:
{!neverExpires && ( <span className="text-xs text-custom-text-400"> {expiredAt === "custom" ? customDate ? `Expires ${renderFormattedDate(customDate)}` : null : expiredAt - ? `Expires ${getExpiryDate(expiredAt ?? "")}` + ? (() => { + const expiryDate = getExpiryDate(expiredAt ?? ""); + return expiryDate ? `Expires ${expiryDate}` : null; + })() : null} </span> )}apiserver/plane/app/serializers/base.py (3)
Line range hint
93-110: Consider using a set for many-to-many field names.The list of fields requiring
many=Trueis growing and could be better organized. Consider using a class-level set for better performance and readability.class DynamicBaseSerializer(BaseSerializer): MANY_TO_MANY_FIELDS = { "members", "assignees", "labels", "issue_cycle", "issue_relation", "issue_inbox", "issue_reactions", "issue_attachment", "issue_link", "sub_issues", "issue_related", } def _filter_fields(self, fields): # ... many = field in self.MANY_TO_MANY_FIELDS self.fields[field] = expansion[field](many=many)
Line range hint
140-161: Refactor to_representation for better maintainability and error handling.The method is handling multiple responsibilities and could benefit from being broken down into smaller, focused methods. Additionally, error handling could be improved when accessing attributes.
Consider breaking down the method:
class DynamicBaseSerializer(BaseSerializer): def _expand_field(self, instance, field_name, expansion_map): try: value = getattr(instance, field_name) serializer_class = expansion_map[field_name] return serializer_class(value, many=isinstance(value, list)).data except AttributeError: return getattr(instance, f"{field_name}_id", None) except Exception as e: logger.error(f"Error expanding field {field_name}: {str(e)}") return None def to_representation(self, instance): response = super().to_representation(instance) if not self.expand: return response expansion_map = get_serializer_map() for expand in self.expand: if expand in self.fields and expand in expansion_map: response[expand] = self._expand_field(instance, expand, expansion_map) return response
Line range hint
1-161: Changes approved with recommendations for improvement.The changes to add
RelatedIssueSerializerand update the expansion logic are functionally correct. However, consider implementing the suggested improvements:
- Extract serializer imports and mappings to a separate module
- Use class-level constants for configuration
- Break down complex methods into smaller, focused ones
- Improve error handling
These changes will make the code more maintainable and robust while reducing duplication.
apiserver/plane/app/urls/issue.py (1)
318-322: Consider grouping bulk operation endpoints together.While the endpoint is well-structured, consider moving it near other bulk operation endpoints (around line 90-100) for better code organization and maintainability.
# Move this block: - path( - "workspaces/<str:slug>/projects/<uuid:project_id>/issue-dates/", - IssueBulkUpdateDateEndpoint.as_view(), - name="project-issue-dates", - ), # After the existing bulk operations around line 90: path( "workspaces/<str:slug>/projects/<uuid:project_id>/bulk-archive-issues/", BulkArchiveIssuesEndpoint.as_view(), name="bulk-archive-issues", ), + path( + "workspaces/<str:slug>/projects/<uuid:project_id>/issue-dates/", + IssueBulkUpdateDateEndpoint.as_view(), + name="project-issue-dates", + ),apiserver/plane/app/views/issue/relation.py (1)
297-297: Consider logging ignored conflicts.The
ignore_conflicts=Truein bulk_create silently ignores duplicate relations. Consider logging these for debugging purposes.+from django.db import IntegrityError +from plane.utils.logger import logger + +# Track attempted and created relations +attempted_relations = len(issues) +created_relations = len(issue_relation) + +if created_relations < attempted_relations: + logger.info( + f"Ignored {attempted_relations - created_relations} duplicate relations " + f"for issue {issue_id} in project {project_id}" + )packages/tailwind-config-custom/tailwind.config.js (2)
Line range hint
397-401: Consider documenting gradient variables.The new onboarding gradient backgrounds are added, but their CSS variable definitions (
--gradient-onboarding-*) should be documented for consistency.Consider adding a comment block above the
backgroundImagesection to document the expected values of these CSS variables:+ // Onboarding gradient variables should be defined as CSS gradients + // Example: --gradient-onboarding-100: linear-gradient(to right, #color1, #color2) backgroundImage: { "onboarding-gradient-100": "var( --gradient-onboarding-100)", "onboarding-gradient-200": "var( --gradient-onboarding-200)", "onboarding-gradient-300": "var( --gradient-onboarding-300)", },
Line range hint
397-401: Fix whitespace in gradient variable references.There's an extra space after
var(in the gradient definitions which should be removed for consistency.backgroundImage: { - "onboarding-gradient-100": "var( --gradient-onboarding-100)", - "onboarding-gradient-200": "var( --gradient-onboarding-200)", - "onboarding-gradient-300": "var( --gradient-onboarding-300)", + "onboarding-gradient-100": "var(--gradient-onboarding-100)", + "onboarding-gradient-200": "var(--gradient-onboarding-200)", + "onboarding-gradient-300": "var(--gradient-onboarding-300)", },apiserver/plane/db/models/issue.py (1)
282-283: LGTM! Consider adding temporal constraint validations.The new relation types "start_before" and "finish_before" are well-named and align with the timeline/Gantt chart requirements.
Consider implementing the following validations in your model or service layer:
- Prevent circular dependencies (e.g., A start_before B, B start_before A)
- Validate conflicting temporal constraints
- Handle cascading updates when related issue dates change
Example validation in the model:
def clean(self): super().clean() if self.relation_type in ['start_before', 'finish_before']: # Prevent circular dependencies if IssueRelation.objects.filter( issue=self.related_issue, related_issue=self.issue, relation_type=self.relation_type ).exists(): raise ValidationError( f"Circular {self.relation_type} dependency detected" ) # Validate temporal constraints if self.relation_type == 'start_before': if (self.issue.start_date and self.related_issue.start_date and self.issue.start_date > self.related_issue.start_date): raise ValidationError( "Issue cannot start after its dependent issue" )web/core/components/gantt-chart/blocks/block.tsx (1)
76-76: Ensure placeholder height matchesBLOCK_HEIGHTThe placeholder's height is set to
h-8, which may not correspond to theBLOCK_HEIGHTused for the block's height. To maintain visual consistency, consider usingBLOCK_HEIGHTfor the placeholder's height.Apply this diff:
- placeholderChildren={<div className="h-8 w-full bg-custom-background-80 rounded" />} + placeholderChildren={ + <div + style={{ height: `${BLOCK_HEIGHT}px` }} + className="w-full bg-custom-background-80 rounded" + /> + }web/core/components/gantt-chart/views/helpers.ts (1)
21-24: SimplifygetNumberOfDaysInMonthfunction.The function can be simplified by returning the result directly without creating an intermediate
datevariable.Apply this diff to simplify the function:
-export const getNumberOfDaysInMonth = (month: number, year: number) => { - const date = new Date(year, month + 1, 0); - return date.getDate(); -}; +export const getNumberOfDaysInMonth = (month: number, year: number) => new Date(year, month + 1, 0).getDate();web/core/components/gantt-chart/blocks/block-row.tsx (1)
35-65: Verify the completeness of the dependency array inuseEffectIn the
useEffecthook, the dependency array is[block], but the effect relies on properties likeblock.positionandblock.id. If these sub-properties can change withoutblockitself changing (e.g., ifblockis a mutable object), the effect may not run as expected.Consider the following options:
If
blockis immutable or its reference changes when any property changes, the current dependency array is sufficient.If properties like
block.positioncan change withoutblock's reference changing, include them in the dependency array:- }, [block]); + }, [block, block.position, block.id]);This ensures the effect runs whenever any of the dependencies change.
web/core/components/gantt-chart/views/quarter-view.ts (4)
102-102: Correct typographical errors in the commentThere's a typo in the comment on line 102: "og" should be "of", and "it's" should be "its".
Apply this diff to fix the comment:
- * Group array of Months into Quarters, returns an array og Quarters and it's children Months + * Group array of Months into Quarters, returns an array of Quarters and its children Months
37-37: Simplify condition by removing unnecessary null checksThe condition
if (minusDate && plusDate)is unnecessary sinceminusDateandplusDateare always definedDateobjects. You can remove this condition to simplify the code.Apply this diff:
- if (minusDate && plusDate) filteredDates = getMonthsBetweenTwoDates(minusDate, plusDate); + filteredDates = getMonthsBetweenTwoDates(minusDate, plusDate);
57-57: Simplify condition by removing unnecessary null checksSimilarly, the condition
if (minusDate && plusDate)can be removed as both variables are always defined.Apply this diff:
- if (minusDate && plusDate) filteredDates = getMonthsBetweenTwoDates(minusDate, plusDate); + filteredDates = getMonthsBetweenTwoDates(minusDate, plusDate);
72-72: Simplify condition by removing unnecessary null checksAgain, remove the unnecessary condition
if (minusDate && plusDate)to simplify the code.Apply this diff:
- if (minusDate && plusDate) filteredDates = getMonthsBetweenTwoDates(minusDate, plusDate); + filteredDates = getMonthsBetweenTwoDates(minusDate, plusDate);web/core/components/gantt-chart/views/month-view.ts (1)
48-48: Remove Redundant Condition ChecksThe conditions
if (minusDate && plusDate)at lines 48, 66, and 80 are redundant becauseminusDateandplusDateare always defined asDateobjects and thus always truthy. Consider removing these checks to simplify the code.Also applies to: 66-66, 80-80
web/core/components/gantt-chart/views/week-view.ts (1)
97-99: Rename 'monthData' to 'weekData' for clarityIn the context of the week view, using
monthDataas a variable name can be confusing. Renaming it toweekDataimproves readability and consistency.Apply this diff to rename the variable:
- const scrollWidth = - filteredDates - .map((monthData: any) => monthData.children.length) + const scrollWidth = + filteredDates + .map((weekData: any) => weekData.children.length)web/core/components/gantt-chart/chart/root.tsx (1)
134-136: Clarify arithmetic expression with parenthesesTo improve readability and ensure correct operator precedence, consider adding parentheses to the
scrollWidthcalculation:scrollWidth = (Math.abs(daysDifference) * currentState.data.dayWidth) - ((clientVisibleWidth / 2) - currentState.data.dayWidth) + (SIDEBAR_WIDTH / 2);This clarifies the intended order of operations.
web/core/components/gantt-chart/chart/main-content.tsx (2)
Line range hint
82-85: Avoid including mutable refs in useEffect dependency arrayIncluding
ganttContainerRef?.currentin the dependency array of theuseEffecthook may cause unnecessary re-runs of the effect because changes toref.currentdo not trigger re-renders. Since refs are mutable and their.currentproperty doesn't cause a re-render when it changes, it's best to exclude it from the dependencies.Apply this diff to fix the dependency array:
- }, [ganttContainerRef?.current]); + }, []);
133-133: Provide context for non-removable IDThe comment
// DO NOT REMOVE THE IDindicates the importance of theidattribute on the<div>. Adding a brief explanation about why thisidshould not be removed will help future maintainers understand its necessity.For example:
- // DO NOT REMOVE THE ID + // DO NOT REMOVE THE ID: Required for automated testingweb/ce/store/timeline/base-timeline.store.ts (4)
184-185: Remove unnecessary optional chaining onblockDatapropertiesAfter the null check at line 177 (
if (!blockData) continue;),blockDatais guaranteed to be defined. The use of optional chaining (blockData?.start_date) on lines 184-185 and 191-192 is unnecessary and inconsistent with how other properties are accessed (e.g.,blockData.name).Apply this diff to improve consistency:
// Lines 184-185 - start_date: blockData?.start_date ?? undefined, - target_date: blockData?.target_date ?? undefined, + start_date: blockData.start_date ?? undefined, + target_date: blockData.target_date ?? undefined, // Lines 191-192 - start_date: blockData?.start_date ?? undefined, - target_date: blockData?.target_date ?? undefined, + start_date: blockData.start_date ?? undefined, + target_date: blockData.target_date ?? undefined,Also applies to: 191-192
245-245: Correct typo in comment: "wile" should be "while"There's a typographical error in the comment for
updateBlockPosition.Apply this diff to fix the typo:
- * updates the block's position such as marginLeft and width wile dragging + * updates the block's position such as marginLeft and width while dragging
218-218: Correct typo in comment: "updates dates" should be "updated dates"The comment for
getUpdatedPositionAfterDragcontains a typo.Apply this diff to correct the comment:
- * returns updates dates of blocks post drag. + * returns updated dates of blocks post drag.
62-62: Consider removing underscore prefix from constructor parameterThe parameter
_rootStorein the constructor uses an underscore prefix, which is typically reserved for denoting private variables. Since the parameter is being assigned to an instance property, consider removing the underscore for clarity and consistency.Apply this diff:
- constructor(_rootStore: CoreRootStore) { + constructor(rootStore: CoreRootStore) { makeObservable(this, { // ... }); this.initGantt(); - this.rootStore = _rootStore; + this.rootStore = rootStore; }apiserver/plane/app/views/issue/base.py (1)
1127-1160: Prevent duplicate entries inissues_to_updatelistAn issue may be appended to
issues_to_updatemultiple times if bothstart_dateandtarget_dateare provided in the update. Whilebulk_updatecan handle duplicates, it's more efficient to ensure each issue is added only once.Consider modifying the code to append the issue after processing both dates:
issues_to_update = [] for update in updates: # existing code... updated = False if start_date: # existing code... issue.start_date = start_date updated = True if target_date: # existing code... issue.target_date = target_date updated = True if updated: issues_to_update.append(issue)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (82)
- apiserver/plane/app/serializers/base.py (4 hunks)
- apiserver/plane/app/urls/issue.py (3 hunks)
- apiserver/plane/app/views/init.py (1 hunks)
- apiserver/plane/app/views/issue/base.py (1 hunks)
- apiserver/plane/app/views/issue/relation.py (3 hunks)
- apiserver/plane/bgtasks/issue_activities_task.py (3 hunks)
- apiserver/plane/db/models/issue.py (1 hunks)
- apiserver/plane/utils/issue_relation_mapper.py (1 hunks)
- packages/tailwind-config-custom/tailwind.config.js (1 hunks)
- packages/types/src/dashboard.d.ts (1 hunks)
- packages/types/src/issues/issue.d.ts (2 hunks)
- packages/types/src/issues/issue_relation.d.ts (1 hunks)
- packages/types/src/view-props.d.ts (1 hunks)
- web/ce/components/gantt-chart/dependency/blockDraggables/index.ts (1 hunks)
- web/ce/components/gantt-chart/dependency/blockDraggables/left-draggable.tsx (1 hunks)
- web/ce/components/gantt-chart/dependency/blockDraggables/right-draggable.tsx (1 hunks)
- web/ce/components/gantt-chart/dependency/dependency-paths.tsx (1 hunks)
- web/ce/components/gantt-chart/dependency/draggable-dependency-path.tsx (1 hunks)
- web/ce/components/gantt-chart/dependency/index.ts (1 hunks)
- web/ce/components/gantt-chart/index.ts (1 hunks)
- web/ce/constants/gantt-chart.ts (1 hunks)
- web/ce/constants/index.ts (1 hunks)
- web/ce/constants/issues.ts (1 hunks)
- web/ce/store/timeline/base-timeline.store.ts (1 hunks)
- web/ce/types/gantt-chart.ts (1 hunks)
- web/ce/types/index.ts (1 hunks)
- web/core/components/api-token/modal/form.tsx (1 hunks)
- web/core/components/core/render-if-visible-HOC.tsx (2 hunks)
- web/core/components/cycles/gantt-chart/blocks.tsx (0 hunks)
- web/core/components/cycles/gantt-chart/cycles-list-layout.tsx (0 hunks)
- web/core/components/cycles/gantt-chart/index.ts (0 hunks)
- web/core/components/cycles/index.ts (0 hunks)
- web/core/components/dashboard/widgets/issue-panels/issue-list-item.tsx (0 hunks)
- web/core/components/gantt-chart/blocks/block-row-list.tsx (1 hunks)
- web/core/components/gantt-chart/blocks/block-row.tsx (1 hunks)
- web/core/components/gantt-chart/blocks/block.tsx (1 hunks)
- web/core/components/gantt-chart/blocks/blocks-list.tsx (1 hunks)
- web/core/components/gantt-chart/chart/header.tsx (2 hunks)
- web/core/components/gantt-chart/chart/main-content.tsx (4 hunks)
- web/core/components/gantt-chart/chart/root.tsx (6 hunks)
- web/core/components/gantt-chart/chart/timeline-drag-helper.tsx (1 hunks)
- web/core/components/gantt-chart/chart/views/bi-week.tsx (0 hunks)
- web/core/components/gantt-chart/chart/views/day.tsx (0 hunks)
- web/core/components/gantt-chart/chart/views/hours.tsx (0 hunks)
- web/core/components/gantt-chart/chart/views/index.ts (0 hunks)
- web/core/components/gantt-chart/chart/views/month.tsx (1 hunks)
- web/core/components/gantt-chart/chart/views/quarter.tsx (1 hunks)
- web/core/components/gantt-chart/chart/views/week.tsx (1 hunks)
- web/core/components/gantt-chart/chart/views/year.tsx (0 hunks)
- web/core/components/gantt-chart/constants.ts (1 hunks)
- web/core/components/gantt-chart/contexts/index.tsx (1 hunks)
- web/core/components/gantt-chart/data/index.ts (2 hunks)
- web/core/components/gantt-chart/helpers/add-block.tsx (3 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/helpers/draggable.tsx (1 hunks)
- web/core/components/gantt-chart/hooks/index.ts (0 hunks)
- web/core/components/gantt-chart/hooks/use-gantt-chart.ts (0 hunks)
- web/core/components/gantt-chart/index.ts (0 hunks)
- web/core/components/gantt-chart/root.tsx (4 hunks)
- web/core/components/gantt-chart/sidebar/cycles/block.tsx (0 hunks)
- web/core/components/gantt-chart/sidebar/cycles/index.ts (0 hunks)
- web/core/components/gantt-chart/sidebar/cycles/sidebar.tsx (0 hunks)
- web/core/components/gantt-chart/sidebar/index.ts (0 hunks)
- web/core/components/gantt-chart/sidebar/issues/block.tsx (2 hunks)
- web/core/components/gantt-chart/sidebar/issues/sidebar.tsx (3 hunks)
- web/core/components/gantt-chart/sidebar/modules/block.tsx (1 hunks)
- web/core/components/gantt-chart/sidebar/modules/sidebar.tsx (1 hunks)
- web/core/components/gantt-chart/sidebar/root.tsx (0 hunks)
- web/core/components/gantt-chart/types/index.ts (3 hunks)
- web/core/components/gantt-chart/views/bi-week-view.ts (0 hunks)
- web/core/components/gantt-chart/views/day-view.ts (0 hunks)
- web/core/components/gantt-chart/views/helpers.ts (2 hunks)
- web/core/components/gantt-chart/views/hours-view.ts (0 hunks)
- web/core/components/gantt-chart/views/index.ts (1 hunks)
- web/core/components/gantt-chart/views/month-view.ts (2 hunks)
- web/core/components/gantt-chart/views/quarter-view.ts (1 hunks)
- web/core/components/gantt-chart/views/quater-view.ts (0 hunks)
- web/core/components/gantt-chart/views/week-view.ts (1 hunks)
- web/core/components/gantt-chart/views/year-view.ts (0 hunks)
- web/core/components/issues/issue-detail-widgets/relations/content.tsx (1 hunks)
⛔ Files not processed due to max files limit (25)
- web/core/components/issues/issue-detail-widgets/relations/quick-action-button.tsx
- web/core/components/issues/issue-detail/issue-activity/activity/actions/relation.tsx
- web/core/components/issues/issue-detail/relation-select.tsx
- web/core/components/issues/issue-layouts/gantt/base-gantt-root.tsx
- web/core/components/issues/relations/issue-list-item.tsx
- web/core/components/issues/relations/issue-list.tsx
- web/core/components/modules/gantt-chart/modules-list-layout.tsx
- web/core/components/ui/loader/layouts/gantt-layout-loader.tsx
- web/core/hooks/use-auto-scroller.tsx
- web/core/hooks/use-timeline-chart.ts
- web/core/layouts/auth-layout/project-wrapper.tsx
- web/core/services/issue/issue.service.ts
- web/core/services/issue/issue_relation.service.ts
- web/core/store/issue/helpers/base-issues.store.ts
- web/core/store/issue/helpers/issue-filter-helper.store.ts
- web/core/store/issue/issue-details/relation.store.ts
- web/core/store/issue/issue-details/root.store.ts
- web/core/store/root.store.ts
- web/core/store/timeline/index.ts
- web/core/store/timeline/issues-timeline.store.ts
- web/core/store/timeline/modules-timeline.store.ts
- web/ee/components/gantt-chart/index.ts
- web/ee/store/timeline/base-timeline.store.ts
- web/helpers/date-time.helper.ts
- web/helpers/issue.helper.ts
💤 Files with no reviewable changes (23)
- web/core/components/cycles/gantt-chart/blocks.tsx
- web/core/components/cycles/gantt-chart/cycles-list-layout.tsx
- web/core/components/cycles/gantt-chart/index.ts
- web/core/components/cycles/index.ts
- web/core/components/dashboard/widgets/issue-panels/issue-list-item.tsx
- web/core/components/gantt-chart/chart/views/bi-week.tsx
- web/core/components/gantt-chart/chart/views/day.tsx
- web/core/components/gantt-chart/chart/views/hours.tsx
- web/core/components/gantt-chart/chart/views/index.ts
- web/core/components/gantt-chart/chart/views/year.tsx
- web/core/components/gantt-chart/hooks/index.ts
- web/core/components/gantt-chart/hooks/use-gantt-chart.ts
- web/core/components/gantt-chart/index.ts
- web/core/components/gantt-chart/sidebar/cycles/block.tsx
- web/core/components/gantt-chart/sidebar/cycles/index.ts
- web/core/components/gantt-chart/sidebar/cycles/sidebar.tsx
- web/core/components/gantt-chart/sidebar/index.ts
- web/core/components/gantt-chart/sidebar/root.tsx
- web/core/components/gantt-chart/views/bi-week-view.ts
- web/core/components/gantt-chart/views/day-view.ts
- web/core/components/gantt-chart/views/hours-view.ts
- web/core/components/gantt-chart/views/quater-view.ts
- web/core/components/gantt-chart/views/year-view.ts
✅ Files skipped from review due to trivial changes (9)
- packages/types/src/issues/issue_relation.d.ts
- web/ce/components/gantt-chart/dependency/blockDraggables/index.ts
- web/ce/components/gantt-chart/dependency/dependency-paths.tsx
- web/ce/components/gantt-chart/dependency/draggable-dependency-path.tsx
- web/ce/components/gantt-chart/dependency/index.ts
- web/ce/components/gantt-chart/index.ts
- web/ce/constants/index.ts
- web/ce/constants/issues.ts
- web/core/components/gantt-chart/views/index.ts
🧰 Additional context used
🪛 Ruff
apiserver/plane/app/views/__init__.py
133-133:
.issue.base.IssueDetailEndpointimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
134-134:
.issue.base.IssueBulkUpdateDateEndpointimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
🪛 GitHub Check: lint-web
web/ce/components/gantt-chart/dependency/blockDraggables/left-draggable.tsx
[warning] 9-9:
'props' is defined but never usedweb/ce/components/gantt-chart/dependency/blockDraggables/right-draggable.tsx
[warning] 8-8:
'props' is defined but never used
🪛 Biome
web/ce/store/timeline/base-timeline.store.ts
[error] 187-187: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
web/core/components/gantt-chart/blocks/block-row-list.tsx
[error] 27-34: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
web/core/components/gantt-chart/helpers/blockResizables/use-gantt-resizable.ts
[error] 107-107: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
web/core/components/gantt-chart/views/week-view.ts
[error] 151-151: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
🔇 Additional comments (81)
web/ce/types/index.ts (1)
3-3: LGTM! Verify type imports in the new Gantt chart views.The addition of the Gantt chart module export aligns well with the Timeline layout revamp objectives. The change follows TypeScript best practices for module organization.
Let's verify the Gantt chart type imports in the new view components:
✅ Verification successful
Types are properly exported and used across the Gantt chart components
The verification confirms that:
- The
gantt-chart.tsexportsTIssueRelationTypeswhich is correctly re-exported throughindex.ts- The view components (week, month, quarter) properly import their required types from the types module
- The type system is well-organized with:
- Common types imported via
../types- View-specific interfaces defined in their respective files
- Proper type sharing between views (e.g.,
IMonthBlockused in quarter view)The barrel file export addition is working as intended, maintaining a clean type organization across the Gantt chart implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the new Timeline views are properly importing types from the Gantt chart module # Expected: Find imports from "../types" or "../../types" in the new view components # Search for Timeline view components and their type imports rg -g '*.tsx' -g '*.ts' -A 3 'import.*from.*types' | rg -C 3 '(Week|Month|Quarter)View'Length of output: 1104
Script:
#!/bin/bash # Let's verify the actual implementation and imports in the view components # to ensure they're properly using the exported types # First, check the content of the gantt-chart types file echo "=== Checking gantt-chart.ts types ===" fd "gantt-chart.ts" --type f --exec cat {} # Then, check the implementation of view components echo -e "\n=== Checking view implementations ===" fd "(week|month|quarter)-view.ts" --type f --exec cat {}Length of output: 16349
web/core/components/gantt-chart/constants.ts (1)
3-3: Verify the impact of reduced header height.The header height reduction from 60px to 48px aligns with modern UI trends, but we should ensure this change doesn't affect the layout negatively.
✅ Verification successful
Header height change appears safe to implement
The reduced HEADER_HEIGHT (48px) is consistently used across all timeline views (Week, Month, Quarter) and the sidebar component. No hardcoded 60px heights were found that could conflict with this change. The constant is properly encapsulated within the Gantt chart components and its usage is uniform throughout the implementation.
- Used in:
- web/core/components/gantt-chart/sidebar/root.tsx
- web/core/components/gantt-chart/chart/views/week.tsx
- web/core/components/gantt-chart/chart/views/month.tsx
- web/core/components/gantt-chart/chart/views/quarter.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for components that might be affected by the HEADER_HEIGHT change rg -l "HEADER_HEIGHT" --type ts --type tsx # Check for any hardcoded height values that might conflict rg "height: *60" --type ts --type tsxLength of output: 138
Script:
#!/bin/bash # Search for components that might be affected by the HEADER_HEIGHT change rg -l "HEADER_HEIGHT" # Check for any hardcoded height values that might conflict rg "height: *60" # Look for style definitions that might use this constant rg "height.*HEADER_HEIGHT"Length of output: 832
web/ce/constants/gantt-chart.ts (2)
1-1: LGTM! Clean type import.The import statement follows TypeScript best practices by explicitly importing the type.
3-8:⚠️ Potential issueVerify alignment with backend relation types.
The backend has added new relation types "start_before" and "finish_before" in
IssueRelationclass, but these aren't reflected in theREVERSE_RELATIONSmapping. This could lead to inconsistencies in relation handling.Let's verify the relation types in the backend:
Consider adding the new relation types with their corresponding reverse relations:
export const REVERSE_RELATIONS: { [key in TIssueRelationTypes]: TIssueRelationTypes } = { blocked_by: "blocking", blocking: "blocked_by", relates_to: "relates_to", duplicate: "duplicate", + start_before: "finish_after", + finish_before: "start_after", };Note: Please verify the correct reverse relations for the new types with the backend team.
web/core/components/gantt-chart/contexts/index.tsx (2)
1-1: LGTM! Clean and focused imports.The imports are minimal and only include the necessary React hooks.
3-6: Verify the completeness of timeline types.The enum
ETimeLineTypeTypecurrently definesISSUEandMODULEtypes, but the PR objectives mention Week, Month, and Quarter views. Could you clarify if:
- These are different concerns (content type vs. view type)?
- Additional timeline types need to be added to support the new views?
Let's check for any related timeline type definitions:
web/core/components/gantt-chart/chart/timeline-drag-helper.tsx (2)
9-11: LGTM! Type definition is clear and well-structured.The Props interface is concise and uses appropriate TypeScript types.
16-16: Verify the auto-scrolling behavior across different views.Since this PR introduces multiple timeline views (Week, Month, Quarter), ensure that the auto-scrolling behavior works correctly across all views.
✅ Verification successful
Let me analyze the results from the shell scripts to verify the auto-scrolling behavior across different views.
The results show that:
- The
useAutoScrollerhook is view-agnostic and takes the same parameters regardless of the view being used.SIDEBAR_WIDTHandHEADER_HEIGHTare constants defined inconstants.tsand are used consistently across all views (Week, Month, Quarter).- The auto-scrolling behavior is implemented in a view-independent manner, as it only depends on container dimensions and scroll positions.
Auto-scrolling implementation is view-independent and works consistently across all views.
The auto-scrolling behavior is correctly implemented to work with any view because:
- It uses fixed constants (
SIDEBAR_WIDTH = 360px,HEADER_HEIGHT = 48px) that are shared across all views- The scrolling logic in
useAutoScrollerhook operates on container dimensions and scroll positions, which are view-agnostic- All views (Week, Month, Quarter) use the same container structure and dimensions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for view-specific configurations or adjustments in auto-scrolling # Test: Look for any view-specific logic in the auto-scroller hook rg -A 5 "useAutoScroller" # Test: Check for any view-specific constants or configurations rg -A 5 "SIDEBAR_WIDTH|HEADER_HEIGHT"Length of output: 17561
web/core/components/gantt-chart/types/index.ts (4)
30-30: LGTM! View types align with PR objectives.The simplified view options ("week", "month", "quarter") directly match the PR requirements for the Timeline layout revamp.
51-51: LGTM! Improved property naming.The rename from
widthtodayWidthbetter describes the property's purpose and improves code clarity.
37-37: Verify the usage of the new abbreviation field.The addition of the
abbreviationfield suggests enhanced display options. Ensure it's being used appropriately in responsive layouts or space-constrained views.#!/bin/bash # Check where and how the abbreviation field is being used ast-grep --pattern 'abbreviation'
4-4: Verify date type changes across the application.The change from
Datetostringtype forstart_dateandtarget_datecould impact date handling across the application. Ensure that:
- All consumers of these fields handle string dates correctly
- Date parsing and formatting are consistent throughout the application
Also applies to: 10-11
✅ Verification successful
Based on the code analysis, I can now provide a final response:
The date type change from
Datetostringis consistent with the codebase's date handlingThe change from
Datetostringtype in the Gantt chart interface is correct and aligns with the codebase's date handling patterns:
- The Gantt chart components consistently handle dates as strings and convert them to Date objects only when needed for calculations using the
getDate()utility- All date-related operations in the Gantt chart use string-based dates that get formatted/parsed using utilities like
renderFormattedPayloadDate()- The type change maintains consistency with other interfaces in the codebase that also use string dates (e.g.
IBlockUpdateData,IBlockUpdateDependencyData)The change improves type safety by:
- Avoiding timezone/serialization issues with Date objects
- Matching the actual runtime data format used throughout the application
- Maintaining consistency with the rest of the codebase's date handling approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct usage of start_date and target_date properties # to verify date handling consistency echo "Checking usage of start_date and target_date in the codebase:" rg -A 3 "start_date|target_date" --type ts --type tsx echo "Checking date parsing/formatting utilities:" rg -A 3 "new Date\(|Date\." --type ts --type tsxLength of output: 519
Script:
#!/bin/bash # Search for direct usage of start_date and target_date properties # to verify date handling consistency echo "Checking usage of start_date and target_date in the codebase:" rg -A 3 "start_date|target_date" echo -e "\nChecking date parsing/formatting utilities:" rg -A 3 "new Date\(|Date\." echo -e "\nChecking for files related to Gantt chart:" fd "gantt" --type fLength of output: 373323
Script:
#!/bin/bash # Search for date handling in Gantt chart related files echo "Checking date handling in Gantt chart files:" rg -A 3 "start_date|target_date" web/core/components/gantt-chart/ echo -e "\nChecking Gantt chart type definitions:" rg -A 3 "interface.*Block|type.*Block" web/core/components/gantt-chart/Length of output: 9420
web/core/components/gantt-chart/blocks/blocks-list.tsx (2)
16-29: LGTM! Clean component structure.Good use of React Fragment to avoid unnecessary DOM nesting. The props destructuring is clean and matches the type definition perfectly.
30-48: Verify showAllBlocks behavior across different timeline views.The
showAllBlocksprop appears to control block visibility, which is crucial for the new Week/Month/Quarter views. Let's verify its implementation across different views.✅ Verification successful
Let me gather more specific information about the showAllBlocks implementation.
Based on the search results, I can now provide a conclusive response about the showAllBlocks behavior.
showAllBlocksimplementation is correctly handling block visibility across viewsThe verification shows that
showAllBlocksis properly implemented:
- In
GanttChartBlock, blocks are hidden if they don't have both start and target dates whenshowAllBlocksis false- The prop is consistently passed down through the component hierarchy from
GanttChartRootto individual blocks- The visibility logic is uniform across all views (Week/Month/Quarter) as it's handled at the block level
- The implementation follows a clear pattern:
if (!block || (!showAllBlocks && !(block.start_date && block.target_date))) return nullThis confirms that the
showAllBlocksbehavior is working as intended for controlling block visibility across different timeline views.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how showAllBlocks is used across different timeline views # Search for showAllBlocks usage in timeline-related components echo "Checking showAllBlocks usage in timeline views..." rg -l 'showAllBlocks.*Week|Month|Quarter' web/ # Check for view-specific rendering logic echo "Checking view-specific rendering conditions..." ast-grep --pattern 'showAllBlocks ? $$$'Length of output: 1986
Script:
#!/bin/bash # Check the implementation of showAllBlocks in GanttChartBlock and related components echo "Checking GanttChartBlock implementation..." rg -A 5 "showAllBlocks" web/core/components/gantt-chart/ # Check how showAllBlocks affects block visibility echo "Checking block visibility logic..." ast-grep --pattern 'const $_ = ({ showAllBlocks, $$$ }) => { $$$ }' web/core/components/gantt-chart/ # Check view-specific implementations echo "Checking view implementations..." rg -A 10 "export (class|const) (Week|Month|Quarter)View" web/core/components/gantt-chart/views/Length of output: 13872
web/core/components/gantt-chart/sidebar/modules/sidebar.tsx (2)
34-44: LGTM! Performance improvement in block handling.The refactoring improves performance by avoiding redundant
getBlockByIdcalls within the map function. The direct passing ofblockIdalso promotes better unidirectional data flow.
42-42: Verify ModulesSidebarBlock implementation.Ensure that
ModulesSidebarBlockproperly handles theblockIdprop and implements necessary memoization to prevent unnecessary re-renders.✅ Verification successful
ModulesSidebarBlock implementation is properly handling props and optimized
The component is well-implemented with the following characteristics:
- Properly handles the
blockIdprop to fetch and display block data- Uses MobX
observerfor efficient reactive updates- Implements proper null checks and conditional rendering
- Has mouse event handlers for block activation state
- Renders duration information only when needed
The component doesn't need explicit memoization because:
- It's already wrapped with MobX
observerwhich optimizes re-renders- Props are simple primitive values (
blockIdandisDragging)- All state management is handled through MobX store
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify ModulesSidebarBlock implementation and its usage of blockId # Test: Check ModulesSidebarBlock implementation ast-grep --pattern 'const ModulesSidebarBlock = $_($_) { $$$ }' # Test: Check for React.memo usage rg -A 5 'memo.*ModulesSidebarBlock'Length of output: 105
Script:
#!/bin/bash # Let's try different approaches to locate ModulesSidebarBlock implementation # Search for ModulesSidebarBlock definition with different patterns rg -A 20 "ModulesSidebarBlock.*=.*\(" # Search for any file containing ModulesSidebarBlock fd ModulesSidebarBlock # Search for exports containing ModulesSidebarBlock rg "export.*ModulesSidebarBlock" # Search for any usage of ModulesSidebarBlock rg "ModulesSidebarBlock"Length of output: 2655
Script:
#!/bin/bash # Let's check the Props interface and any potential performance optimizations # Check Props interface definition rg -B 10 "interface Props" web/core/components/gantt-chart/sidebar/modules/block.tsx # Check if there are any performance related hooks or memoization rg -A 5 "useMemo|useCallback|memo" web/core/components/gantt-chart/sidebar/modules/block.tsx # Check observer implementation since we see it's used rg "observer" web/core/components/gantt-chart/sidebar/modules/block.tsxLength of output: 366
Script:
#!/bin/bash # Let's get the complete file content to analyze the implementation thoroughly # Get the full content of block.tsx cat web/core/components/gantt-chart/sidebar/modules/block.tsx # Check how the store is implemented since it's using MobX rg -A 10 "useTimeLineChartStore" web/core/components/gantt-chart/sidebar/modules/block.tsxLength of output: 2712
web/core/components/gantt-chart/sidebar/modules/block.tsx (1)
1-16: LGTM! Clean imports and improved prop interface.The transition from passing the entire block object to just the
blockIdis a good practice that reduces prop drilling and centralizes state management.web/core/components/gantt-chart/blocks/block-row-list.tsx (1)
27-34: Great use of virtualization for performance optimization!The implementation of
RenderIfVisiblewith appropriate offset values ensures smooth scrolling and optimal performance by only rendering rows that are visible in the viewport.🧰 Tools
🪛 Biome
[error] 27-34: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
web/core/components/gantt-chart/helpers/blockResizables/left-resizable.tsx (4)
1-18: LGTM! Well-structured type definitions and imports.The props interface is well-defined with clear types and the imports are properly organized.
19-23: LGTM! Clean component setup with proper state management.The component correctly implements the observer pattern and uses appropriate state management for UI interactions.
28-30: LGTM! Clear resize state handling.The resize state logic is straightforward and the early return pattern is well applied.
24-26: Verify date handling for edge cases.While the date calculation logic looks correct, consider adding error handling for invalid dates or positions.
Run this script to check for potential edge cases in date handling across the codebase:
✅ Verification successful
Based on the script results, I can now generate the final response since I have enough information about the date handling implementation across the codebase.
Date handling implementation is properly safeguarded
The current implementation already has sufficient null checks and edge case handling:
- The
positioncheck prevents undefined access:position ? getDateFromPositionOnGantt(...) : undefined- The store implementation has a guard for missing view data:
if (!this.currentViewData) return- The date formatting is safely handled:
date ? renderFormattedDate(date) : undefined🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of getDateFromPositionOnGantt to understand edge case handling rg -A 3 "getDateFromPositionOnGantt" # Search for any existing error handling patterns for date calculations ast-grep --pattern 'getDateFromPositionOnGantt($_)'Length of output: 3970
web/core/components/gantt-chart/helpers/blockResizables/right-resizable.tsx (1)
10-18:⚠️ Potential issueConsider making the position prop required or add runtime validation.
The optional
positionprop is used directly in calculations (line 25) without comprehensive null checking. This could lead to runtime errors if the prop is undefined.Let's verify the usage of this component to see if position is always provided:
Consider either:
- Making the position prop required:
type RightResizableProps = { enableBlockRightResize: boolean; handleBlockDrag: (e: React.MouseEvent<HTMLDivElement, MouseEvent>, dragDirection: "left" | "right" | "move") => void; isMoving: "left" | "right" | "move" | undefined; - position?: { + position: { marginLeft: number; width: number; }; };
- Or adding runtime validation:
if (!position) { console.warn('Position prop is required for proper date calculation'); return null; }web/core/components/gantt-chart/chart/header.tsx (2)
3-12: LGTM! Well-organized imports with clear grouping.The import changes align with the Timeline layout revamp, and the organization follows good practices with clear grouping using comments.
29-29: Verify consistent hook usage across Gantt chart components.The switch from
useGanttCharttouseTimeLineChartStorelooks good, but let's verify consistent usage across related components.✅ Verification successful
Hook usage is consistent across Gantt chart components
The verification shows that:
- No instances of
useGanttChartremain in the codebaseuseTimeLineChartStoreis consistently imported from "@/hooks/use-timeline-chart" across all Gantt chart components- The hook is properly used to access various store properties and methods throughout the components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that useGanttChart has been replaced with useTimeLineChartStore across Gantt chart components echo "Checking for any remaining useGanttChart usage..." rg "useGanttChart" "web/core/components/gantt-chart" echo "Verifying useTimeLineChartStore implementation in Gantt chart components..." rg "useTimeLineChartStore" "web/core/components/gantt-chart"Length of output: 4501
packages/types/src/issues/issue.d.ts (1)
59-60: Clarify the distinction between issue_relation and issue_related.The purpose of having two separate arrays for relations isn't immediately clear. The naming could be more explicit about the direction of the relationships.
Let's check if these fields are used consistently in the codebase:
Consider renaming these fields to be more explicit about their roles:
- issue_relation?: IssueRelation[]; - issue_related?: IssueRelation[]; + outgoing_relations?: IssueRelation[]; + incoming_relations?: IssueRelation[];web/core/components/gantt-chart/root.tsx (3)
Line range hint
31-52: Clean component implementation with proper defaults!The observer HOC is correctly applied, and the props destructuring with sensible defaults enhances maintainability.
63-84: Clean and efficient prop forwarding!The render logic maintains a clear separation of concerns by passing all necessary props to ChartViewRoot without unnecessary manipulation.
Line range hint
1-86: Verify integration with new Timeline views.Since this PR introduces Week, Month, and Quarter views, we should verify that this Gantt chart component properly integrates with all three views.
web/core/components/core/render-if-visible-HOC.tsx (2)
16-16: LGTM: Clean prop type additionThe
forceRenderoptional boolean prop is well-defined and properly integrated into the Props type.
33-33: LGTM: Proper default value assignmentThe default value of
falseforforceRendermaintains the original behavior by default.web/core/components/gantt-chart/helpers/add-block.tsx (3)
11-14: LGTM! Good architectural improvement.The transition to
useTimeLineChartStoreindicates a move towards more centralized state management, which is beneficial for managing multiple timeline views.
64-65: 🛠️ Refactor suggestionOptimize mousemove handler to prevent unnecessary recreations.
The current implementation may cause performance issues due to the circular dependency between the mousemove handler and buttonXPosition.
- }, [buttonXPosition, currentViewData]); + }, [currentViewData]); // Remove buttonXPosition dependencyLikely invalid or redundant comment.
41-43:⚠️ Potential issueReview the numberOfDays logic for different views.
The current implementation has several potential issues:
- Only quarter view is explicitly handled
- Week and month views default to 1 day, which might not align with user expectations
- The magic number 7 for quarter view lacks explanation
Consider implementing view-specific spans:
- let numberOfDays = 1; - if (currentView === "quarter") numberOfDays = 7; + const VIEW_SPANS = { + week: 1, + month: 3, + quarter: 7 + }; + const numberOfDays = VIEW_SPANS[currentView] ?? 1;✅ Verification successful
The previous search was too specific. Let's broaden our search to understand the view-related logic better and verify if the current implementation aligns with the codebase's patterns.
The output shows that there's a
VIEWS_LISTbeing used with the view types, and we need to understand if there are any existing configurations for time spans. This will help us verify if the current implementation aligns with the codebase's patterns.
The current numberOfDays implementation is correct and consistent with the codebase
The implementation aligns with the codebase's design where:
- Week view uses 1 day as the default span (confirmed by week-view.ts implementation showing individual day blocks)
- Month view uses 1 day as it operates on week-based blocks (7 days) internally
- Quarter view uses 7 days which matches the larger time scale and grid implementation in quarter-view.ts
The current implementation is intentionally simple and works with the view-specific rendering logic in the respective view components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any configurations or constants defining these view-specific spans rg -A 2 "DEFAULT_SPAN" || rg -A 2 "VIEW_SPANS"Length of output: 43
Script:
#!/bin/bash # Let's search for view-related constants and configurations rg -g '*.{ts,tsx,js,jsx}' -B 2 -A 2 'currentView.*=.*("week"|"month"|"quarter")' # Also search for any view-related types or interfaces ast-grep --pattern 'type $_ = "week" | "month" | "quarter"' # Check the component's props or type definitions fd -e ts -e tsx gantt-chart --exec rg -l "type.*Props" {}Length of output: 2304
Script:
#!/bin/bash # Let's check the VIEWS_LIST constant and any related view configurations rg -g '*.{ts,tsx}' -B 2 -A 5 'VIEWS_LIST.*=' # Also search for any time-span or day-related configurations in the gantt chart rg -g '*.{ts,tsx}' 'days?' web/core/components/gantt-chart/Length of output: 18657
web/core/components/gantt-chart/sidebar/issues/block.tsx (2)
11-11: LGTM: Good architectural decision to use centralized timeline store.The switch to
useTimeLineChartStorealigns well with the timeline revamp, providing a centralized state management solution that can support all timeline views (Week, Month, Quarter).
27-27: Verify duration calculations across different timeline views.The switch to position-based duration calculation using
getNumberOfDaysFromPositionneeds verification across Week, Month, and Quarter views to ensure consistent behavior.Let's verify the implementation:
✅ Verification successful
Let me gather more information about the timeline store implementation to understand how the position-based calculations work.
Let me try different search patterns to locate the store implementation.
Let me search for the implementation of
getNumberOfDaysFromPositionto understand how it handles different timeline views.
Let me try different search patterns to find the timeline implementation.
Based on the code found, I can now generate a final response:
Position-based duration calculation is view-independent and consistent.
The implementation of
getNumberOfDaysFromPositioninbase-timeline.store.tsshows that the duration calculation is consistent across all views (week/month/quarter) since it uses the view-specificdayWidthfromcurrentViewData. The calculation is:
- Simply divides the position width by the current view's
dayWidthvalue- Uses
Math.round()to handle any floating-point precision issues- Returns undefined for invalid inputs (no position or no view data)
This ensures accurate duration calculations regardless of the selected timeline view, as each view provides its own appropriate
dayWidthscaling factor.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of getNumberOfDaysFromPosition to understand the calculation logic ast-grep --pattern 'getNumberOfDaysFromPosition($_)' # Look for test files covering this functionality fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts timelineLength of output: 451
Script:
#!/bin/bash # Search for the store implementation to understand the calculation logic ast-grep --pattern 'getNumberOfDaysFromPosition = $_' # Look for the timeline store implementation rg -l "useTimeLineChartStore" --type ts --type tsx # Search for view-related logic in the store rg "view.*timeline" --type ts --type tsx -A 3Length of output: 210
Script:
#!/bin/bash # Search for the store implementation rg "getNumberOfDaysFromPosition" -A 5 # Look for timeline store files fd timeline store # Search for view-related constants or types rg "type.*View.*=|enum.*View" -A 3Length of output: 292426
Script:
#!/bin/bash # Search for the timeline store implementation rg -l "useTimeLineChartStore" --type ts --type tsx -A 3 # Search for timeline view-related code rg "currentViewData" --type ts --type tsx -A 3 # Search for timeline view types rg "type.*View.*=.*week.*month.*quarter" --type ts --type tsxLength of output: 250
Script:
#!/bin/bash # Search for timeline store and view-related code rg "getNumberOfDaysFromPosition" -A 5 # Search for timeline view types rg "type.*TGanttViews" -A 3 # Search for timeline store implementation fd timeline storeLength of output: 3734
web/core/components/gantt-chart/data/index.ts (2)
30-34: LGTM! Quarter data structure is well-implemented.The quarters data structure follows a consistent pattern and provides clear quarter representations with appropriate titles and abbreviations.
Line range hint
1-94: Implementation successfully supports Timeline layout revamp.The changes effectively support the PR objectives by providing well-structured data and configurations for Week, Month, and Quarter views. The implementation maintains consistency and provides appropriate customization options for each view type.
web/core/components/gantt-chart/sidebar/issues/sidebar.tsx (4)
8-17: LGTM: Import changes align with performance objectives.The addition of
RenderIfVisibleandGanttLayoutLIstItemcomponents supports the PR's performance improvement goals through optimized rendering.
92-107: LGTM: Drag and drop implementation.The drag-and-drop functionality is well-maintained within the virtualized list, preserving the existing interaction patterns while benefiting from the performance improvements.
84-90: Verify RenderIfVisible offset values.The horizontal (100) and vertical (200) offset values seem arbitrary. These values determine how far from the viewport edges we start rendering items.
#!/bin/bash # Description: Check for consistent offset values across similar components # Search for other RenderIfVisible usage to compare offset values rg -A 2 "horizontalOffset.*verticalOffset" "web/core"
47-47: Verify consistent hook usage across components.The migration from prop-based to hook-based block retrieval is a significant architectural change.
✅ Verification successful
Hook migration is consistent across the codebase
The verification shows that:
- No interfaces or prop destructuring patterns for
getBlockByIdwere found, indicating complete migration away from prop-based approachuseTimeLineChartwithETimeLineTypeType.ISSUEis consistently used in issue-related components- The hook is properly integrated into the component hierarchy through the store context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that other components have also migrated to using useTimeLineChart # for block retrieval instead of receiving getBlockById as a prop # Search for any remaining prop-based usage rg "getBlockById.*Props" "web/core/components" # Search for hook adoption rg "useTimeLineChart.*getBlockById" "web/core/components"Length of output: 510
Script:
#!/bin/bash # Let's try a more comprehensive search strategy # 1. Search for getBlockById in props or interfaces ast-grep --pattern 'interface $_ { $$$ getBlockById$$$ $$$ }' # 2. Search for getBlockById prop destructuring ast-grep --pattern '{ $$$getBlockById$$$}' # 3. Search for useTimeLineChart hook usage rg "useTimeLineChart" -A 2 "web/core" # 4. Search specifically for the ISSUE enum usage with the hook rg "useTimeLineChart.*ISSUE" "web/core"Length of output: 13079
web/core/components/gantt-chart/chart/views/month.tsx (3)
14-16: LGTM! Clean implementation of the new store hook.The transition to
useTimeLineChartStorewith proper typing looks good.
18-26: LGTM! Good error handling and clean data preparation.The null check and date calculations are well implemented. The code is defensive and handles edge cases appropriately.
89-102: Verify week column widths across different screen sizes.The week columns implementation looks good, but let's ensure it works well across different screen sizes and zoom levels.
Let's check for any responsive design issues:
✅ Verification successful
Based on the search results, I can now generate the final response as we have sufficient information about the width handling in the Gantt chart component.
Week column widths implementation is correctly handled
The week columns implementation uses a robust width calculation system:
- Width is dynamically calculated using
currentViewData?.data.dayWidth * 7for each week block- The component is part of a larger responsive system with proper scroll handling and container width management
- Width calculations are consistently applied across different views (week, month, quarter)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for responsive design related code and viewport handling rg -A 3 "useResizeObserver|useWindowSize|@media|viewport" web/core/components/gantt-chart/Length of output: 92
Script:
#!/bin/bash # Let's check for responsive design patterns and width calculations in the gantt chart component rg -A 5 "width|resize|screen|responsive|zoom" web/core/components/gantt-chart/chart/Length of output: 6143
Script:
#!/bin/bash # Let's also check for any viewport or dimension-related hooks/utilities fd . web/core/components/gantt-chart/ -e ts -e tsx --exec rg -l "useRef|useState|useEffect.*width|dimension|viewport|screen|resize"Length of output: 792
Script:
#!/bin/bash # Check the parent component structure and any container width handling ast-grep --pattern 'const $_ = $$$; return (<div $$$>{$$$}</div>)' web/core/components/gantt-chart/chart/views/month.tsxLength of output: 122
packages/types/src/view-props.d.ts (1)
81-82: LGTM: Addition of "expand" parameterThe addition of the "expand" parameter to TIssueParams aligns with the enhanced issue relation handling mentioned in the AI summary.
apiserver/plane/app/views/__init__.py (1)
133-134: Verify the imports are properly registered in URL configurations.The newly added endpoints align well with the Timeline Layout revamp requirements. However, let's ensure they are properly registered and accessible.
✅ Verification successful
The imports are properly registered and used in the URL configurations
The verification confirms that both endpoints are:
- Defined in
apiserver/plane/app/views/issue/base.py- Imported and re-exported in
apiserver/plane/app/views/__init__.py- Properly registered in URL patterns in
apiserver/plane/app/urls/issue.py🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new endpoints are registered in URL configurations # and check for any existing usages. # Test 1: Check URL configuration echo "Checking URL patterns..." rg -l "IssueDetailEndpoint|IssueBulkUpdateDateEndpoint" "apiserver/plane/app/urls/" # Test 2: Check for any existing usage echo "Checking for endpoint usage..." rg "IssueDetailEndpoint|IssueBulkUpdateDateEndpoint" --type pyLength of output: 894
🧰 Tools
🪛 Ruff
133-133:
.issue.base.IssueDetailEndpointimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
134-134:
.issue.base.IssueBulkUpdateDateEndpointimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
apiserver/plane/app/urls/issue.py (2)
24-26: LGTM! Clean import additions.The new imports are properly organized and follow the existing pattern.
45-49: LGTM! Well-structured endpoint definition.The URL pattern follows RESTful conventions and maintains consistency with other endpoints in the file.
apiserver/plane/app/views/issue/relation.py (3)
41-41: LGTM: Good practice to centralize relation type mapping.The addition of
get_actual_relationutility function helps standardize relation type handling across the codebase.
273-277: LGTM: Good error handling for relation_type.The early validation with clear error message is a good practice.
235-266: Verify relation type consistency.The response data structure assumes that relation types are consistently mapped (e.g., "start_before" maps to "start_after"). Consider adding validation to ensure this consistency.
packages/tailwind-config-custom/tailwind.config.js (4)
292-292: LGTM: New font size addition aligns with Timeline layout needs.The addition of the "2xs" (9px) font size provides better control for compact UI elements in the Timeline views.
Line range hint
308-396: Consistent spacing scale maintains visual harmony.The 90% scaling has been consistently applied across all spacing utilities (padding, margin, space, gap). The mathematical calculations are accurate, and the values maintain proper proportions.
Line range hint
308-396: Verify the impact of spacing changes on responsive layouts.The global reduction in spacing could affect component layouts and responsive behavior, particularly in dense UI areas like the Timeline.
#!/bin/bash # Search for components using spacing utilities echo "Checking components using spacing utilities..." rg -l "\b(p|m|space|gap)-[0-9]" --type ts --type tsx # Look for responsive design implementations echo "Checking responsive design patterns..." rg -l "@media|sm:|md:|lg:|xl:" --type ts --type tsx
Line range hint
292-307: Verify the impact of font size scaling on existing components.While scaling down font sizes to 90% makes sense for the Timeline layout, this global change could affect readability in other parts of the application.
✅ Verification successful
Based on the search results, I can now provide a final response:
Font size scaling appears to be a deliberate design choice with widespread usage
The codebase shows extensive usage of text size utilities across numerous components (200+ files), and the font size scaling is part of a comprehensive design system where spacing, padding, and other related properties are consistently scaled to 90%. This appears to be an intentional design decision that maintains proportional relationships across the UI.
Key observations:
- The scaling is applied uniformly across the entire design system
- Text utilities are used consistently throughout the application
- No accessibility-related issues or guidelines were found that would conflict with this scaling
- The scaled sizes maintain readable proportions (e.g., base font at 0.9rem is still within acceptable readability ranges)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing font size utility usage that might be affected echo "Checking components using font size utilities..." rg -l "text-\b(xs|sm|base|lg|xl|2xl|3xl|4xl|5xl|6xl|7xl|8xl|9xl)\b" --type ts --type tsx # Check for any accessibility-related font size requirements in documentation echo "Checking for accessibility guidelines..." fd -e md -e mdx | xargs rg -l "font.*size|text.*size|accessibility"Length of output: 449
Script:
#!/bin/bash # Search for existing font size utility usage that might be affected echo "Checking components using font size utilities..." rg -l "text-\b(xs|sm|base|lg|xl|2xl|3xl|4xl|5xl|6xl|7xl|8xl|9xl)\b" # Look for any existing font size customizations in Tailwind configs echo "Checking other Tailwind configurations..." fd tailwind.config.js | xargs cat # Check for any accessibility-related font size requirements in documentation echo "Checking for accessibility guidelines..." fd README.md | xargs rg -i "font|text|accessibility"Length of output: 58994
apiserver/plane/bgtasks/issue_activities_task.py (2)
34-34: LGTM! Good separation of concerns.The addition of
get_inverse_relationimport improves code organization by centralizing the relation mapping logic in a dedicated utility module.
Line range hint
1398-1412: Improved maintainability with centralized relation mapping.The changes enhance the code by:
- Using
get_inverse_relationutility to determine inverse relationships- Removing hardcoded relation type mapping
- Making the code more maintainable and consistent
Let's verify the implementation of the inverse relation mapper:
✅ Verification successful
Centralized relation mapping implementation verified successfully
The code changes are well-implemented with a centralized relation mapping in
apiserver/plane/utils/issue_relation_mapper.py. Theget_inverse_relationfunction maintains a comprehensive bidirectional mapping for all relation types:
- start_after ↔ start_before
- finish_after ↔ finish_before
- blocked_by ↔ blocking
The implementation is being used correctly in the issue activities task to maintain consistency in bidirectional relationships between issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of get_inverse_relation function # Test: Search for the function definition and its usage ast-grep --pattern 'def get_inverse_relation($_) { $$$ }'Length of output: 61
Script:
#!/bin/bash # Let's try searching for the function definition using ripgrep rg -p "def get_inverse_relation" -A 10 # Also search for any relation type mapping or constants rg -p "RELATION_TYPE|relation_type" -A 5Length of output: 15132
web/core/components/gantt-chart/helpers/draggable.tsx (3)
1-11: Imports and dependencies are correctly structuredThe imported modules are appropriate and necessary for the functionality of the component.
50-52: EnsurehandleBlockDragis properly invoked on block movementThe
onMouseDownevent handler correctly checksenableBlockMovebefore invokinghandleBlockDrag. This prevents unintended drag actions when moving is disabled.
37-62: Component structure and rendering logic are appropriateThe structure of the component uses the new
LeftResizableandRightResizablecomponents effectively, improving modularity and separation of concerns.web/core/components/gantt-chart/views/helpers.ts (3)
Line range hint
28-39: Verify correctness of week number calculation ingetWeekNumberByDate.The current implementation of
getWeekNumberByDatemay not accurately calculate the ISO week number for all dates due to variations in week definitions across different locales.Would you like assistance in verifying this function or refactoring it to use a standard library like
date-fnsfor reliable week number calculations?
117-120: Great job on structuring the timeline view helpers.Exporting the
timelineViewHelperswith the views for week, month, and quarter enhances modularity and makes it easier to manage different timeline perspectives.
95-95:⚠️ Potential issueHandle zero value in
positionDaysDifference.In the
getItemPositionWidthfunction, the checkif (!positionDaysDifference) return;will cause the function to return whenpositionDaysDifferenceis zero. This means items starting on the chart's start date won't be rendered.Consider modifying the condition to properly handle zero:
- if (!positionDaysDifference) return; + if (positionDaysDifference < 0) return;This ensures that items with a valid
positionDaysDifferenceof zero are processed correctly.Likely invalid or redundant comment.
web/core/components/gantt-chart/chart/views/week.tsx (2)
13-14: LGTMThe state management using
useTimeLineChartStoreand the mapping ofrenderViewtoweekBlocksis appropriate and aligns with the overall architecture.
12-12: 🛠️ Refactor suggestionAvoid using
anytype in component definitionUsing
anyas the props type in the component can lead to a loss of type safety. Specify the appropriate props interface for theWeekChartViewcomponent to enhance type checking and maintainability.Apply this diff to define and use a specific props interface:
+ interface WeekChartViewProps { + // Define the props used by WeekChartView here, or use an empty object if there are none. + } - export const WeekChartView: FC<any> = observer(() => { + export const WeekChartView: FC<WeekChartViewProps> = observer(() => {Likely invalid or redundant comment.
web/core/components/gantt-chart/blocks/block-row.tsx (2)
50-51: Prevent unnecessary state updates in the IntersectionObserver callbackThe
setIsHiddenandsetIsBlockHiddenOnLeftfunctions are called on every intersection change, even if the values haven't changed, potentially causing redundant re-renders.[performance]
Before updating the state, compare the new values with the current state to prevent unnecessary updates:
- setIsHidden(!entry.isIntersecting); - setIsBlockHiddenOnLeft(entry.boundingClientRect.left < 0); + const newIsHidden = !entry.isIntersecting; + if (isHidden !== newIsHidden) { + setIsHidden(newIsHidden); + } + const newIsBlockHiddenOnLeft = entry.boundingClientRect.left < 0; + if (isBlockHiddenOnLeft !== newIsBlockHiddenOnLeft) { + setIsBlockHiddenOnLeft(newIsBlockHiddenOnLeft); + }
84-121: Overall implementation is robust and well-structuredThe
BlockRowcomponent is well-designed, with clear state management and effective use of hooks. The component handles conditional rendering and user interactions efficiently.web/core/components/gantt-chart/helpers/blockResizables/use-gantt-resizable.ts (1)
1-124: Overall implementation is sound and well-structuredThe
useGanttResizablehook is well-implemented, effectively managing the resizing and dragging functionality for Gantt chart blocks. The use ofuseRefanduseStateprovides efficient state management, and event listeners are appropriately added and removed to prevent memory leaks.🧰 Tools
🪛 Biome
[error] 107-107: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
web/core/components/gantt-chart/views/quarter-view.ts (1)
123-124: Ensure 'quarterNumber' is within valid range when accessing 'quarters' arrayAfter adjusting
quarterNumberto be 1-based, verify that accessingquarters[quarterNumber - 1]is within the valid index range of thequartersarray. This prevents potentialundefinederrors.Consider adding a check to ensure
quarterNumber - 1is not out of bounds:const index = quarterNumber - 1; if (index >= 0 && index < quarters.length) { const quarterData = quarters[index]; // existing code } else { // Handle invalid quarterNumber }web/core/components/gantt-chart/views/month-view.ts (2)
33-90: Well-Structured Date Handling ingenerateMonthChartThe
generateMonthChartfunction efficiently handles date ranges based on thesideparameter, enhancing the flexibility and maintainability of the month view generation.
141-153: Confirm Uniqueness in Merged PayloadsIn the
mergeMonthRenderPayloadsfunction, months and weeks are merged usinguniqBywith keys based on month-year and week start-end times. Ensure that these keys reliably prevent duplicates, especially during edge cases like daylight saving time changes or time zone differences.Run the following script to check for potential duplicate entries:
✅ Verification successful
Based on the code analysis, I can now provide a final response:
The Uniqueness Keys are Properly Implemented
The uniqueness implementation is reliable and handles edge cases appropriately:
- For months: The key
${monthBlock.month}_${monthBlock.year}is sufficient as months are identified by their numerical month and year values, which are timezone-independent.- For weeks: The key
${weekBlock.startDate.getTime()}_${weekBlock.endDate.getTime()}uses Unix timestamps (milliseconds since epoch) which are timezone-safe and DST-aware, ensuring unique identification of week blocks.The codebase shows consistent timestamp handling across the gantt chart component, with proper date arithmetic in related files (helpers.ts, week-view.ts) supporting this implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that merged months and weeks have unique keys. # Test: List all month keys and check for duplicates. echo "Month keys:" node -e " const months = [...]; // Replace with actual months array const keys = months.map(m => \`\${m.month}_\${m.year}\`); console.log(keys); " # Test: List all week keys and check for duplicates. echo "Week keys:" node -e " const weeks = [...]; // Replace with actual weeks array const keys = weeks.map(w => \`\${w.startDate.getTime()}_\${w.endDate.getTime()}\`); console.log(keys); "Length of output: 534
Script:
#!/bin/bash # Let's analyze the implementation and usage of mergeMonthRenderPayloads # First, let's find the interface definition for IMonthView rg -t typescript "interface IMonthView" -A 10 # Then, let's find all usages of mergeMonthRenderPayloads rg -t typescript "mergeMonthRenderPayloads" # Let's also check for any test files that might validate this functionality fd -e test.ts -e spec.ts | xargs rg "mergeMonthRenderPayloads" # Finally, let's look for any timezone or date-related utility functions that might be used rg -t typescript "getTime|timezone|UTC" -A 2 web/core/components/gantt-chart/Length of output: 574
Script:
#!/bin/bash # Let's try without typescript file type specification # Find the interface definition echo "=== IMonthView Interface ===" rg "interface IMonthView" -A 10 web/core/components/gantt-chart/ # Look for the implementation context echo "=== mergeMonthRenderPayloads Context ===" rg "mergeMonthRenderPayloads" -B 5 -A 10 web/core/components/gantt-chart/ # Check for date/time handling echo "=== Date/Time Usage ===" rg "getTime|timezone|UTC" -A 2 web/core/components/gantt-chart/ # Look for any tests or specs echo "=== Test Files Content ===" find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) -exec grep -l "monthView\|IMonthView" {} \; | xargs catLength of output: 4917
web/core/components/gantt-chart/chart/root.tsx (6)
7-9: LGTM: Updated import touseTimeLineChartStoreThe import statement has been correctly updated to include the new hook.
12-13: LGTM: Added necessary type importsThe necessary types and helpers have been correctly imported from
"../types"and"../views".
32-32: AddedupdateBlockDatesprop toChartViewRootPropsThe optional
updateBlockDatesprop has been appropriately added to the component's props.
59-59: DestructuredupdateBlockDatesfrom propsThe
updateBlockDatesprop is correctly destructured fromprops, ensuring it can be utilized within the component.
66-66: Updated to useuseTimeLineChartStorehookThe component now utilizes the
useTimeLineChartStorehook, reflecting the updated state management approach.
176-176: PropagatedupdateBlockDatestoGanttChartMainContentThe
updateBlockDatesfunction is correctly passed down toGanttChartMainContent, ensuring that block dates can be updated as needed.web/core/components/gantt-chart/chart/main-content.tsx (2)
96-103: Verify the logic in 'onScroll' function calculationsThe calculation of
approxRangeLeftmay not behave as intended. The conditionscrollLeft >= clientWidth + 1000might not correctly determine when to update the view. Review the logic to ensure thatapproxRangeLeftandapproxRangeRightcorrectly reflect when to callupdateCurrentViewRenderPayload.Consider checking the calculations with different scroll positions to confirm their correctness.
121-121: Confirm 'ganttContainerRef' compatibility in 'TimelineDragHelper'The
TimelineDragHelpercomponent receivesganttContainerRefas a prop. Verify that this ref aligns with the expected type and usage withinTimelineDragHelperto ensure proper functionality.web/ce/store/timeline/base-timeline.store.ts (1)
284-285:⚠️ Potential issueImplement logic for
getIsCurrentDependencyDraggingor remove placeholderThe method
getIsCurrentDependencyDraggingcurrently returnsfalsefor all inputs, suggesting it's a placeholder. Leaving it unimplemented may cause confusion or unintended behavior. If this method is not yet needed, consider removing it until it's ready to be implemented, or add a TODO comment indicating future work.
| @@ -0,0 +1 @@ | |||
| export type TIssueRelationTypes = "blocking" | "blocked_by" | "duplicate" | "relates_to"; | |||
There was a problem hiding this comment.
💡 Codebase verification
The temporal relation types are not used in the frontend Gantt chart implementation
Based on the evidence:
- Backend
RELATION_CHOICESexplicitly includesstart_beforeandfinish_beforerelations - These temporal relations are actively used in the backend API (
plane/app/views/issue/relation.py) - The frontend type
TIssueRelationTypesis missing these temporal relations - The
REVERSE_RELATIONSconstant inweb/ce/constants/gantt-chart.tsrelies on this type, making temporal relations crucial for Gantt chart functionality
Please update the type definition to include all relation types:
-export type TIssueRelationTypes = "blocking" | "blocked_by" | "duplicate" | "relates_to";
+export type TIssueRelationTypes = "blocking" | "blocked_by" | "duplicate" | "relates_to" | "start_before" | "finish_before";🔗 Analysis chain
Add missing temporal relation types for timeline functionality.
The type definition appears to be missing temporal relation types that exist in the backend model (start_before and finish_before). Given that this PR focuses on timeline views, these temporal relations could be crucial for proper Gantt chart functionality.
Apply this diff to include all relation types:
-export type TIssueRelationTypes = "blocking" | "blocked_by" | "duplicate" | "relates_to";
+export type TIssueRelationTypes = "blocking" | "blocked_by" | "duplicate" | "relates_to" | "start_before" | "finish_before";Let's verify the relation types used in the codebase:
Let me gather more information about the actual relation types defined in the backend models.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of temporal relation types in the codebase
# Expected: Find instances where these relation types might be needed
# Check for temporal relation patterns in TypeScript/JavaScript files
echo "Checking for temporal relation patterns:"
rg -t ts -t js "start_before|finish_before"
# Check for relation type definitions in Python files
echo -e "\nChecking backend relation definitions:"
rg -t py "RELATION_CHOICES|relation_type"
Length of output: 5420
Script:
#!/bin/bash
# Check the actual relation choices in the models
echo "Checking RELATION_CHOICES definition:"
rg -A 10 "RELATION_CHOICES = \(" apiserver/plane/db/models/issue.py
# Check if these temporal relations are already used in the frontend
echo -e "\nChecking frontend usage of relation types:"
rg -t ts "TIssueRelationTypes" -A 2
Length of output: 14654
| export const TimeLineTypeContext = createContext<ETimeLineTypeType | undefined>(undefined); | ||
|
|
||
| const initializeStore = () => { | ||
| const newGanttViewStore = ganttViewStore ?? new GanttStore(); | ||
| if (typeof window === "undefined") return newGanttViewStore; | ||
| if (!ganttViewStore) ganttViewStore = newGanttViewStore; | ||
| return newGanttViewStore; | ||
| }; | ||
|
|
||
| type GanttStoreProviderProps = { | ||
| children: React.ReactNode; | ||
| }; | ||
| export const useTimeLineType = () => { | ||
| const timelineType = useContext(TimeLineTypeContext); | ||
|
|
||
| export const GanttStoreProvider: FC<GanttStoreProviderProps> = ({ children }) => { | ||
| const store = initializeStore(); | ||
| return <GanttStoreContext.Provider value={store}>{children}</GanttStoreContext.Provider>; | ||
| return timelineType; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety and error handling to the timeline context.
The current implementation could be improved with better type safety and error handling:
Consider this enhanced implementation:
-export const TimeLineTypeContext = createContext<ETimeLineTypeType | undefined>(undefined);
+export const TimeLineTypeContext = createContext<ETimeLineTypeType | undefined>(undefined);
+if (process.env.NODE_ENV !== 'production') {
+ TimeLineTypeContext.displayName = 'TimeLineTypeContext';
+}
export const useTimeLineType = () => {
const timelineType = useContext(TimeLineTypeContext);
+ if (timelineType === undefined) {
+ throw new Error('useTimeLineType must be used within a TimeLineTypeContext.Provider');
+ }
return timelineType;
};This change:
- Adds a descriptive display name for better debugging
- Ensures the hook is used within a provider
- Makes the return type non-nullable, improving type safety
📝 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.
| export const TimeLineTypeContext = createContext<ETimeLineTypeType | undefined>(undefined); | |
| const initializeStore = () => { | |
| const newGanttViewStore = ganttViewStore ?? new GanttStore(); | |
| if (typeof window === "undefined") return newGanttViewStore; | |
| if (!ganttViewStore) ganttViewStore = newGanttViewStore; | |
| return newGanttViewStore; | |
| }; | |
| type GanttStoreProviderProps = { | |
| children: React.ReactNode; | |
| }; | |
| export const useTimeLineType = () => { | |
| const timelineType = useContext(TimeLineTypeContext); | |
| export const GanttStoreProvider: FC<GanttStoreProviderProps> = ({ children }) => { | |
| const store = initializeStore(); | |
| return <GanttStoreContext.Provider value={store}>{children}</GanttStoreContext.Provider>; | |
| return timelineType; | |
| export const TimeLineTypeContext = createContext<ETimeLineTypeType | undefined>(undefined); | |
| if (process.env.NODE_ENV !== 'production') { | |
| TimeLineTypeContext.displayName = 'TimeLineTypeContext'; | |
| } | |
| export const useTimeLineType = () => { | |
| const timelineType = useContext(TimeLineTypeContext); | |
| if (timelineType === undefined) { | |
| throw new Error('useTimeLineType must be used within a TimeLineTypeContext.Provider'); | |
| } | |
| return timelineType; | |
| }; |
| export const TimelineDragHelper = observer((props: Props) => { | ||
| const { ganttContainerRef } = props; | ||
| const { isDragging } = useTimeLineChartStore(); | ||
|
|
||
| useAutoScroller(ganttContainerRef, isDragging, SIDEBAR_WIDTH, HEADER_HEIGHT); | ||
| return <></>; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing component documentation and type safety.
While the implementation is correct, it could benefit from additional documentation and type safety improvements.
+/**
+ * Helper component that enables auto-scrolling functionality during drag operations in the Gantt chart.
+ * @param props.ganttContainerRef - Reference to the Gantt chart container element
+ */
-export const TimelineDragHelper = observer((props: Props) => {
+export const TimelineDragHelper: React.FC<Props> = observer((props) => {
const { ganttContainerRef } = props;
const { isDragging } = useTimeLineChartStore();
useAutoScroller(ganttContainerRef, isDragging, SIDEBAR_WIDTH, HEADER_HEIGHT);
return <></>;
});
+
+TimelineDragHelper.displayName = "TimelineDragHelper";📝 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.
| export const TimelineDragHelper = observer((props: Props) => { | |
| const { ganttContainerRef } = props; | |
| const { isDragging } = useTimeLineChartStore(); | |
| useAutoScroller(ganttContainerRef, isDragging, SIDEBAR_WIDTH, HEADER_HEIGHT); | |
| return <></>; | |
| }); | |
| /** | |
| * Helper component that enables auto-scrolling functionality during drag operations in the Gantt chart. | |
| * @param props.ganttContainerRef - Reference to the Gantt chart container element | |
| */ | |
| export const TimelineDragHelper: React.FC<Props> = observer((props) => { | |
| const { ganttContainerRef } = props; | |
| const { isDragging } = useTimeLineChartStore(); | |
| useAutoScroller(ganttContainerRef, isDragging, SIDEBAR_WIDTH, HEADER_HEIGHT); | |
| return <></>; | |
| }); | |
| TimelineDragHelper.displayName = "TimelineDragHelper"; |
| relation_mapping = { | ||
| "start_after": "start_before", | ||
| "finish_after": "finish_before", | ||
| "blocked_by": "blocking", | ||
| "blocking": "blocked_by", | ||
| "start_before": "start_after", | ||
| "finish_before": "finish_after", | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider defining relation mapping as a module-level constant.
Since the relation mapping is static, it should be defined as a module-level constant to improve maintainability and prevent recreation on each function call.
Consider refactoring to:
+INVERSE_RELATION_MAPPING = {
+ "start_after": "start_before",
+ "finish_after": "finish_before",
+ "blocked_by": "blocking",
+ "blocking": "blocked_by",
+ "start_before": "start_after",
+ "finish_before": "finish_after",
+}
+
def get_inverse_relation(relation_type):
- relation_mapping = {
- "start_after": "start_before",
- "finish_after": "finish_before",
- "blocked_by": "blocking",
- "blocking": "blocked_by",
- "start_before": "start_after",
- "finish_before": "finish_after",
- }
- return relation_mapping.get(relation_type, relation_type)
+ return INVERSE_RELATION_MAPPING.get(relation_type, relation_type)📝 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.
| relation_mapping = { | |
| "start_after": "start_before", | |
| "finish_after": "finish_before", | |
| "blocked_by": "blocking", | |
| "blocking": "blocked_by", | |
| "start_before": "start_after", | |
| "finish_before": "finish_after", | |
| } | |
| INVERSE_RELATION_MAPPING = { | |
| "start_after": "start_before", | |
| "finish_after": "finish_before", | |
| "blocked_by": "blocking", | |
| "blocking": "blocked_by", | |
| "start_before": "start_after", | |
| "finish_before": "finish_after", | |
| } | |
| def get_inverse_relation(relation_type): | |
| return INVERSE_RELATION_MAPPING.get(relation_type, relation_type) |
| def get_actual_relation(relation_type): | ||
| # This function is used to get the actual relation type which is store in database | ||
| actual_relation = { | ||
| "start_after": "start_before", | ||
| "finish_after": "finish_before", | ||
| "blocking": "blocked_by", | ||
| "blocked_by": "blocked_by", | ||
| "start_before": "start_before", | ||
| "finish_before": "finish_before", | ||
| } | ||
|
|
||
| return actual_relation.get(relation_type, relation_type) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve function documentation and consider mapping optimization.
The function has several areas for improvement:
- The current comment should be converted to a proper docstring
- Some mappings appear redundant (e.g., "blocked_by" -> "blocked_by")
- The mapping should be a module-level constant like suggested for get_inverse_relation
Consider this refactoring:
+ACTUAL_RELATION_MAPPING = {
+ "start_after": "start_before",
+ "finish_after": "finish_before",
+ "blocking": "blocked_by",
+ "start_before": "start_before",
+ "finish_before": "finish_before",
+}
+
def get_actual_relation(relation_type):
- # This function is used to get the actual relation type which is store in database
- actual_relation = {
- "start_after": "start_before",
- "finish_after": "finish_before",
- "blocking": "blocked_by",
- "blocked_by": "blocked_by",
- "start_before": "start_before",
- "finish_before": "finish_before",
- }
-
- return actual_relation.get(relation_type, relation_type)
+ """
+ Maps a relation type to its canonical form as stored in the database.
+
+ Args:
+ relation_type (str): The relation type to normalize
+
+ Returns:
+ str: The normalized relation type for database storage
+ """
+ return ACTUAL_RELATION_MAPPING.get(relation_type, relation_type)📝 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.
| def get_actual_relation(relation_type): | |
| # This function is used to get the actual relation type which is store in database | |
| actual_relation = { | |
| "start_after": "start_before", | |
| "finish_after": "finish_before", | |
| "blocking": "blocked_by", | |
| "blocked_by": "blocked_by", | |
| "start_before": "start_before", | |
| "finish_before": "finish_before", | |
| } | |
| return actual_relation.get(relation_type, relation_type) | |
| ACTUAL_RELATION_MAPPING = { | |
| "start_after": "start_before", | |
| "finish_after": "finish_before", | |
| "blocking": "blocked_by", | |
| "start_before": "start_before", | |
| "finish_before": "finish_before", | |
| } | |
| def get_actual_relation(relation_type): | |
| """ | |
| Maps a relation type to its canonical form as stored in the database. | |
| Args: | |
| relation_type (str): The relation type to normalize | |
| Returns: | |
| str: The normalized relation type for database storage | |
| """ | |
| return ACTUAL_RELATION_MAPPING.get(relation_type, relation_type) |
| <ActiveChartView /> | ||
| {currentViewData && ( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider lazy loading 'ActiveChartView' for performance
If ActiveChartView is a large component, utilizing React.lazy and Suspense could improve the initial load performance by code-splitting and lazy loading the component when needed.
Example implementation:
+import React, { lazy, Suspense } from "react";
...
-const ActiveChartView = CHART_VIEW_COMPONENTS[currentView];
+const ActiveChartView = lazy(() => Promise.resolve({ default: CHART_VIEW_COMPONENTS[currentView] }));
...
return (
...
+ <Suspense fallback={<div>Loading...</div>}>
<ActiveChartView />
+ </Suspense>
...
);Committable suggestion was skipped due to low confidence.
| getUpdatedPositionAfterDrag: (id: string, ignoreDependencies?: boolean) => IBlockUpdateDependencyData[]; | ||
| updateBlockPosition: (id: string, deltaLeft: number, deltaWidth: number, ignoreDependencies?: boolean) => void; |
There was a problem hiding this comment.
Ensure method signatures match between interface and implementation
The methods getUpdatedPositionAfterDrag and updateBlockPosition in the IBaseTimelineStore interface include an ignoreDependencies?: boolean parameter, but this parameter is missing in the implementation within BaseTimeLineStore. This discrepancy can lead to type inconsistencies and unexpected behaviors.
Apply this diff to align the method signatures:
// In BaseTimeLineStore class
-getUpdatedPositionAfterDrag = (id: string) => {
+getUpdatedPositionAfterDrag = (id: string, ignoreDependencies?: boolean) => {
-updateBlockPosition = (id: string, deltaLeft: number, deltaWidth: number) => {
+updateBlockPosition = (id: string, deltaLeft: number, deltaWidth: number, ignoreDependencies?: boolean) => {Also applies to: 222-222
| currentView: TGanttViews; | ||
| currentViewData: ChartDataType | undefined; | ||
| activeBlockId: string | null; | ||
| renderView: any; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify a concrete type instead of using any for renderView
The property renderView and its related methods use the any type, which can undermine TypeScript's type safety benefits. Defining a specific type for renderView will enhance code reliability and maintainability.
Consider defining a type for renderView:
// Define a type for renderView data
type RenderViewType = YourSpecificTypeHere;
// Update the property and method signatures
renderView: RenderViewType = [];
updateRenderView: (data: RenderViewType) => void;Also applies to: 58-58, 143-143
|
|
||
| updates = request.data.get("updates", []) | ||
|
|
||
| issue_ids = [update["id"] for update in updates] |
There was a problem hiding this comment.
Add validation for the presence of the "id" key in updates
The code assumes that every update in updates contains an "id" key. If an update is missing this key, a KeyError will be raised. To prevent this, consider adding validation to handle missing "id" keys gracefully.
Apply this diff to fix the issue:
- issue_ids = [update["id"] for update in updates]
+ issue_ids = [update["id"] for update in updates if "id" in update]
...
- issue_id = update["id"]
+ issue_id = update.get("id")
+ if not issue_id:
+ continue # Skip or handle the error appropriatelyAlso applies to: 1108-1108
|
|
||
| # Fetch all relevant issues in a single query | ||
| issues = list(Issue.objects.filter(id__in=issue_ids)) | ||
| issues_dict = {str(issue.id): issue for issue in issues} |
There was a problem hiding this comment.
Ensure consistent types for issue IDs when accessing issues_dict
Currently, issues_dict uses string representations of issue IDs as keys (str(issue.id)), but issue_id from update["id"] may be a UUID object. This type mismatch can lead to KeyError when retrieving issues from the dictionary. Ensure that both the keys and the lookup use consistent types.
Apply this diff to fix the issue:
- issues_dict = {str(issue.id): issue for issue in issues}
+ issues_dict = {issue.id: issue for issue in issues}Also applies to: 1110-1110
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (17)
web/ce/components/relations/index.tsx (1)
6-35: Well-structured relation options with room for enhancement.The implementation provides a type-safe and consistent way to define issue relations with appropriate visual distinctions. However, consider the following improvements:
export const ISSUE_RELATION_OPTIONS: Record<TIssueRelationTypes, TRelationObject> = { relates_to: { key: "relates_to", label: "Relates to", - className: "bg-custom-background-80 text-custom-text-200", + className: "bg-custom-background-80 text-custom-text-200", + description: "Issues that are related but not blocked", + ariaLabel: "Related issue", icon: (size) => <RelatedIcon height={size} width={size} className="text-custom-text-200" />, placeholder: "Add related issues", }, // Apply similar changes to other relation types... };Consider these enhancements:
- Add descriptions to improve documentation
- Include aria-labels for better accessibility
- Consider extracting color classes into theme constants for better maintainability
web/core/components/issues/issue-detail-widgets/relations/quick-action-button.tsx (1)
33-40: LGTM! Consider adding type safety to Object.values.The changes improve the menu's UI with controlled height and use a more idiomatic approach with
Object.values(). The implementation is clean and maintains good separation of concerns.Consider adding explicit typing to improve type safety:
- {Object.values(ISSUE_RELATION_OPTIONS).map((item, index) => ( + {Object.values(ISSUE_RELATION_OPTIONS).map((item: typeof ISSUE_RELATION_OPTIONS[keyof typeof ISSUE_RELATION_OPTIONS], index) => (web/core/components/issues/issue-detail/issue-activity/activity/actions/relation.tsx (1)
25-25: Consider adding null check and extracting icon sizeThe icon rendering logic looks good, but consider these improvements:
- Add a null check for
activity.fieldto prevent potential runtime errors- Extract the hardcoded icon size (14) to a constant or prop for better maintainability
+ const ICON_SIZE = 14; + <IssueActivityBlockComponent - icon={activity.field ? ISSUE_RELATION_OPTIONS[activity.field as TIssueRelationTypes].icon(14) : <></>} + icon={activity.field && ISSUE_RELATION_OPTIONS[activity.field as TIssueRelationTypes]?.icon(ICON_SIZE) || <></>}web/core/components/issues/issue-detail-widgets/issue-detail-widget-collapsibles.tsx (1)
Line range hint
10-15: Consider enhancing type safety for issue-related props.Consider creating a dedicated interface for the component props that includes validation for the ID formats. This would help catch potential issues at compile-time.
interface IssueDetailWidgetProps { workspaceSlug: `${string}-${string}`; // assuming UUID format projectId: string; issueId: string; disabled: boolean; }web/core/components/issues/issue-detail-widgets/relations/helper.tsx (1)
Line range hint
19-95: Consider enhancing error handling and user feedback.The hook implementation is solid, but there are a few areas that could be improved:
- The
removeoperation swallows errors silently- The success/error messages could be more descriptive
- The
copyTextfunction lacks error handlingConsider applying these improvements:
copyText: (text: string) => { const originURL = typeof window !== "undefined" && window.location.origin ? window.location.origin : ""; - copyTextToClipboard(`${originURL}/${text}`).then(() => { - setToast({ - type: TOAST_TYPE.SUCCESS, - title: "Link Copied!", - message: "Issue link copied to clipboard.", - }); - }); + copyTextToClipboard(`${originURL}/${text}`) + .then(() => { + setToast({ + type: TOAST_TYPE.SUCCESS, + title: "Link Copied!", + message: "Issue link copied to clipboard.", + }); + }) + .catch(() => { + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: "Failed to copy issue link to clipboard.", + }); + }); }, // In the update operation setToast({ title: "Success!", type: TOAST_TYPE.SUCCESS, - message: "Issue updated successfully", + message: `Issue ${issueId} updated successfully`, }); // In the remove operation remove: async (workspaceSlug: string, projectId: string, issueId: string) => { try { return removeIssue(workspaceSlug, projectId, issueId).then(() => { captureIssueEvent({ eventName: ISSUE_DELETED, payload: { id: issueId, state: "SUCCESS", element: "Issue detail page" }, path: pathname, }); + setToast({ + type: TOAST_TYPE.SUCCESS, + title: "Success!", + message: `Issue ${issueId} deleted successfully`, + }); }); } catch (error) { captureIssueEvent({ eventName: ISSUE_DELETED, payload: { id: issueId, state: "FAILED", element: "Issue detail page" }, path: pathname, }); + setToast({ + type: TOAST_TYPE.ERROR, + title: "Error!", + message: `Failed to delete issue ${issueId}`, + }); + throw error; // Re-throw to allow caller to handle } },web/core/components/gantt-chart/chart/views/week.tsx (3)
11-11: Add proper typing for component props instead of usingany.Using
FC<any>reduces type safety. Consider creating a proper interface for the component props, even if currently empty.- export const WeekChartView: FC<any> = observer(() => { + interface WeekChartViewProps {} + export const WeekChartView: FC<WeekChartViewProps> = observer(() => {
12-13: Improve variable naming clarity.The variable
renderViewis being assigned toweekBlocks, which suggests thatrenderViewmight not be the most descriptive name in the store. Consider renaming it in the store to match its usage.
16-91: Add ARIA attributes for accessibility.The calendar-like interface should include appropriate ARIA attributes and roles for better accessibility. Consider adding:
role="grid"to the main containerrole="columnheader"to day headersrole="gridcell"to day cells- Appropriate
aria-labelattributes for datesExample for the main container:
- <div className={`absolute top-0 left-0 h-max w-max flex`} + <div + role="grid" + aria-label="Week view calendar" + className={`absolute top-0 left-0 h-max w-max flex`}web/core/components/gantt-chart/chart/views/quarter.tsx (1)
12-13: Add type safety to hook destructuring.Consider adding explicit type annotations to the destructured values from useTimeLineChartStore to ensure type safety throughout the component.
- const { currentViewData, renderView } = useTimeLineChartStore(); + const { + currentViewData, + renderView + }: { + currentViewData: TimelineViewData; + renderView: IMonthBlock[] + } = useTimeLineChartStore();web/core/components/gantt-chart/chart/views/month.tsx (3)
22-25: Consider adding validation for date calculations.While the date calculations look correct, consider adding validation to ensure
monthsandweeksarrays are not empty before accessing their first elements.- const monthsStartDate = new Date(months[0].year, months[0].month, 1); - const weeksStartDate = weeks[0].startDate; + const monthsStartDate = months.length > 0 ? new Date(months[0].year, months[0].month, 1) : new Date(); + const weeksStartDate = weeks.length > 0 ? weeks[0].startDate : new Date();
62-87: Enhance accessibility for week headers.The week headers could benefit from improved accessibility for screen readers.
<div key={`sub-title-${weekBlock.startDate}-${weekBlock.endDate}`} + role="columnheader" + aria-label={`Week ${weekBlock.weekData.shortTitle}, ${weekBlock.startDate.getDate()}-${weekBlock.endDate.getDate()}`} 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-10": weekBlock.today, } )}
89-102: Consider virtualization for better performance.For large datasets, rendering all week columns simultaneously could impact performance. Consider implementing virtualization to render only visible columns.
Consider using a virtualization library like
react-windoworreact-virtualizedto optimize the rendering of week columns. This would be especially beneficial when displaying many weeks of data.web/core/components/issues/issue-detail-widgets/relations/content.tsx (2)
Line range hint
108-108: Fix incomplete gap value in flex container.The className has an incomplete gap value:
gap--<div className="flex flex-col gap-"> +<div className="flex flex-col gap-2">
Line range hint
127-168: Consider extracting modal rendering logic to reduce duplication.The modal rendering logic for delete and update operations follows a similar pattern. Consider extracting this into a reusable component or helper function to improve maintainability.
Example refactor:
type ModalProps = { type: 'delete' | 'update'; state: TIssueCrudState; onClose: () => void; onSubmit: (issue?: TIssue) => Promise<void>; }; const IssueModal: FC<ModalProps> = ({ type, state, onClose, onSubmit }) => { if (!state.toggle || !state.issue) return null; return type === 'delete' ? ( <DeleteIssueModal isOpen={state.toggle} handleClose={onClose} data={state.issue} onSubmit={onSubmit} /> ) : ( <CreateUpdateIssueModal isOpen={state.toggle} onClose={onClose} data={state.issue} onSubmit={onSubmit} /> ); };web/core/store/issue/issue-details/relation.store.ts (3)
1-3: Consolidate Lodash ImportsConsider consolidating your Lodash imports into a single statement to improve code readability and maintainability.
Apply this change:
-import get from "lodash/get"; -import set from "lodash/set"; -import uniq from "lodash/uniq"; +import { get, set, uniq } from "lodash";
42-42: Update Interface DocumentationEnsure that the new methods
getRelationCountByIssueId,extractRelationsFromIssues, andcreateCurrentRelationare fully documented in theIIssueRelationStoreinterface for better clarity and to assist other developers.Also applies to: 44-45
176-222: Improve Error Logging increateCurrentRelationIn the
catchblock, consider logging the error or providing more context to aid in debugging if the API call fails. This can help identify issues more quickly during development.Add error logging:
catch (e) { // Revert back store changes if API fails runInAction(() => { // revert changes }); + console.error("Failed to create current relation:", e); throw e; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
- web/ce/components/relations/index.tsx (1 hunks)
- web/ce/store/timeline/base-timeline.store.ts (1 hunks)
- web/core/components/gantt-chart/chart/views/month.tsx (1 hunks)
- web/core/components/gantt-chart/chart/views/quarter.tsx (1 hunks)
- web/core/components/gantt-chart/chart/views/week.tsx (1 hunks)
- web/core/components/issues/issue-detail-widgets/issue-detail-widget-collapsibles.tsx (1 hunks)
- web/core/components/issues/issue-detail-widgets/relations/content.tsx (3 hunks)
- web/core/components/issues/issue-detail-widgets/relations/helper.tsx (1 hunks)
- web/core/components/issues/issue-detail-widgets/relations/quick-action-button.tsx (2 hunks)
- web/core/components/issues/issue-detail-widgets/relations/title.tsx (1 hunks)
- web/core/components/issues/issue-detail/issue-activity/activity/actions/relation.tsx (2 hunks)
- web/core/components/issues/issue-detail/relation-select.tsx (3 hunks)
- web/core/store/issue/issue-details/relation.store.ts (8 hunks)
- web/ee/components/relations/index.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/ee/components/relations/index.tsx
🧰 Additional context used
🪛 Biome
web/ce/store/timeline/base-timeline.store.ts
[error] 185-185: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (17)
web/core/components/issues/issue-detail-widgets/relations/title.tsx (1)
20-20: LGTM! Good performance optimization.The change from
getRelationsByIssueIdtogetRelationCountByIssueIdis a nice optimization. Instead of retrieving all relations and counting them, we now directly get the count, which is more efficient especially when dealing with large numbers of relations.Also applies to: 24-24
web/ce/components/relations/index.tsx (2)
1-4: LGTM! Well-organized imports.The imports are properly structured, separating third-party libraries, internal UI components, and type definitions.
6-35: Verify the usage of ISSUE_RELATION_OPTIONS across the codebase.Let's ensure this centralized configuration is properly utilized by other components.
✅ Verification successful
ISSUE_RELATION_OPTIONS is properly integrated across the codebase
The configuration is correctly utilized throughout the codebase:
- Used for rendering relation UI elements in
relation-select.tsx- Powers the relation menu options in
quick-action-button.tsx- Drives the relation collapsible sections in
content.tsx- Handles activity tracking in
relation.tsx- Manages relation state and operations in
relation.store.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of ISSUE_RELATION_OPTIONS across the codebase # Search for imports of ISSUE_RELATION_OPTIONS echo "Checking imports of ISSUE_RELATION_OPTIONS:" rg "import.*ISSUE_RELATION_OPTIONS.*from" -A 2 # Search for direct usage of the constant echo "\nChecking usage of ISSUE_RELATION_OPTIONS:" rg "ISSUE_RELATION_OPTIONS\." -A 2Length of output: 2075
Script:
#!/bin/bash # Let's search for actual usage patterns of ISSUE_RELATION_OPTIONS in the files echo "Checking content of relation-select.tsx:" cat "web/core/components/issues/issue-detail/relation-select.tsx" echo -e "\nChecking content of quick-action-button.tsx:" cat "web/core/components/issues/issue-detail-widgets/relations/quick-action-button.tsx" echo -e "\nChecking content of content.tsx:" cat "web/core/components/issues/issue-detail-widgets/relations/content.tsx" echo -e "\nChecking content of relation.tsx:" cat "web/core/components/issues/issue-detail/issue-activity/activity/actions/relation.tsx" echo -e "\nChecking content of relation.store.ts:" cat "web/core/store/issue/issue-details/relation.store.ts"Length of output: 25700
web/core/components/issues/issue-detail/issue-activity/activity/actions/relation.tsx (2)
5-7: LGTM: Improved code organization with centralized importsThe migration to centralized imports from
@/plane-webnamespace improves maintainability by ensuring consistent relation definitions across components.
Line range hint
31-42: Verify handling of all relation typesThe component handles 'blocking', 'blocked_by', 'duplicate', and 'relates_to' relations. Let's verify this covers all possible relation types.
✅ Verification successful
Let me gather more information to locate the definition of relation types.
Let me search in the relations-specific files we found to locate the relation types definition.
Let me try one more search without file type restrictions.
Based on the search results, I can now provide a final response. The code shows that all relation types are properly handled:
All relation types are correctly implemented
The component handles all possible relation types as defined in
web/ce/types/gantt-chart.ts:export type TIssueRelationTypes = "blocking" | "blocked_by" | "duplicate" | "relates_to";The current implementation in
relation.tsxcovers all these types:
- blocking
- blocked_by
- duplicate
- relates_to
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if all relation types from ISSUE_RELATION_OPTIONS are handled in the component # Search for the definition of ISSUE_RELATION_OPTIONS to compare with handled types ast-grep --pattern 'const ISSUE_RELATION_OPTIONS = { $$$ }' # Search for all relation type handling cases in the component rg "activity\.field === \".*?\"" "web/core/components/issues/issue-detail/issue-activity/activity/actions/relation.tsx"Length of output: 354
Script:
#!/bin/bash # Search for any constants or types related to issue relations rg -i "relation|issue.*type" --type typescript -A 3 # Search specifically in the issues directory fd . "web/core/components/issues" -e ts -e tsx --exec rg -l "enum|type|interface.*Relation"Length of output: 19252
Script:
#!/bin/bash # Search in relations-related files for type definitions rg -t tsx -t ts "type.*Relation|enum.*Relation|interface.*Relation|RELATION" web/core/components/issues/relations/ rg -t tsx -t ts "type.*Relation|enum.*Relation|interface.*Relation|RELATION" web/core/components/issues/issue-detail/relation-select.tsxLength of output: 311
Script:
#!/bin/bash # Search for relation type definitions across all files rg "enum.*Relation|RELATION_TYPE.*=|ISSUE_RELATION.*=|type.*Relation.*=|interface.*Relation.*\{" -A 5 # Also search specifically in the constants or types files fd "types|constants" -e ts -e tsx --exec rg "Relation" {}Length of output: 8522
web/core/components/issues/issue-detail-widgets/issue-detail-widget-collapsibles.tsx (3)
27-27: LGTM: Improved performance by using relation count.The change from
getRelationsByIssueIdtogetRelationCountByIssueIdis a good optimization as it avoids fetching unnecessary relation data when only the count is needed.
33-33: LGTM: Clean implementation of relations count.The implementation is clear and maintains a single responsibility of getting the relations count.
37-37: Verify handling of edge cases in relation count.While the simplified condition
issueRelationsCount > 0is cleaner, please verify that the count accurately reflects valid relations. Consider cases where relations might be marked as deleted or invalid but still counted.✅ Verification successful
Let me check the relation validation logic further to understand how relations are handled.
Let me check one more aspect to understand how relations are extracted and validated.
The simplified condition is safe and correctly handles relation validity
The
getRelationCountByIssueIdimplementation already handles validation by:
- Only counting relations that exist in
ISSUE_RELATION_OPTIONS- Relations are properly extracted and validated during
extractRelationsFromIssues- The relation types are strictly typed as "blocking", "blocked_by", "duplicate", or "relates_to"
The count accurately reflects valid relations, making the simplified render condition both performant and correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for relation validation logic rg -A 5 "getRelationCountByIssueId" --type tsLength of output: 3789
Script:
#!/bin/bash # Check relation types and validation ast-grep --pattern 'const ISSUE_RELATION_OPTIONS = { $$$ }' # Check how relations are stored and filtered rg -A 5 "getRelationsByIssueId" --type tsLength of output: 3148
Script:
#!/bin/bash # Check relation extraction and validation rg -A 10 "extractRelationsFromIssues" --type ts # Check relation types rg "type TIssueRelationTypes" -A 5 --type tsLength of output: 5171
web/core/components/issues/issue-detail-widgets/relations/helper.tsx (1)
Line range hint
1-13: LGTM! Clean imports and well-structured type definitions.The imports are properly organized and the type definitions are comprehensive.
web/core/components/gantt-chart/chart/views/quarter.tsx (1)
15-15: Verify edge case handling in groupMonthsToQuarters.Please ensure that the groupMonthsToQuarters function properly handles edge cases such as:
- Empty monthBlocks array
- Months spanning across years
- Incomplete quarters
✅ Verification successful
Let me gather more information about the implementation of
groupMonthsToQuartersfunction.
Let me get more context about the interfaces and the quarters constant being used.
Let me get the complete implementation of groupMonthsToQuarters and the quarters constant definition to verify the edge cases handling.
Let me try to get the quarters constant definition with a different approach.
Based on the implementation details gathered, I can now provide the final response:
The groupMonthsToQuarters function properly handles edge cases
The implementation shows robust handling of edge cases:
- Empty monthBlocks array: Returns an empty array as Object.values() of empty quartersMap
- Months spanning across years: Handled by using unique quarterKey with year (
Q${quarterNumber}-${year})- Incomplete quarters: Quarters are built incrementally as months are processed, so partial quarters are valid
- Current quarter detection: Properly identifies current quarter using todayQuarterNumber and todayYear
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the implementation and tests of groupMonthsToQuarters ast-grep --pattern 'function groupMonthsToQuarters($_) { $$$ }' # Search for any test files related to this function rg -l "groupMonthsToQuarters"Length of output: 207
Script:
#!/bin/bash # Get the implementation details from both files rg -A 20 "groupMonthsToQuarters" web/core/components/gantt-chart/views/quarter-view.ts rg -A 20 "groupMonthsToQuarters" web/core/components/gantt-chart/chart/views/quarter.tsx # Also search for any interfaces or types related to quarters rg "interface.*Quarter" web/core/components/gantt-chart/Length of output: 2405
Script:
#!/bin/bash # Get the interface definition and quarters constant rg -A 10 "interface IQuarterMonthBlock" web/core/components/gantt-chart/views/quarter-view.ts rg -B 5 -A 10 "const quarters" web/core/components/gantt-chart/views/quarter-view.ts # Get the IMonthBlock interface definition rg -A 10 "interface IMonthBlock" web/core/components/gantt-chart/Length of output: 1813
Script:
#!/bin/bash # Get the complete implementation of groupMonthsToQuarters and quarters constant rg -B 10 -A 30 "export const groupMonthsToQuarters" web/core/components/gantt-chart/views/quarter-view.ts # Get the quarters constant definition rg -B 2 -A 10 "const quarters = {" web/core/components/gantt-chart/views/quarter-view.tsLength of output: 1515
Script:
#!/bin/bash # Get the quarters constant definition using a simpler pattern rg "quarters = \{" -A 10 web/core/components/gantt-chart/views/quarter-view.ts # Also check if there are any tests for edge cases rg -l "test|spec|describe.*groupMonthsToQuarters" web/core/components/gantt-chart/Length of output: 165
web/core/components/gantt-chart/chart/views/month.tsx (3)
17-20: LGTM! Proper null checking implemented.Good defensive programming with the null check before destructuring the monthView data.
28-30: LGTM! Clean layout structure.The component layout is well-structured with proper conditional rendering and dynamic height calculation.
3-16: LGTM! Verify hook migration completeness.The migration from
useGanttCharttouseTimeLineChartStorealigns with the Timeline layout revamp. The imports are properly structured with clear separation of concerns.Let's verify that all instances of the old hook have been migrated:
✅ Verification successful
Hook migration is complete and consistent
The migration from
useGanttCharttouseTimeLineChartStorehas been fully completed. The search results show:
- No instances of the old
useGanttCharthook remain in the codebaseuseTimeLineChartStoreis consistently used across all Gantt chart components- The hook is properly defined in
web/core/hooks/use-timeline-chart.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of the old hook rg "useGanttChart" --type ts --type tsxLength of output: 67
Script:
#!/bin/bash # Search for any remaining instances of the old hook using ripgrep without file type filters rg "useGanttChart" # Also search for timeline-related hooks to verify the migration rg "useTimeLineChartStore"Length of output: 4389
web/core/components/issues/issue-detail-widgets/relations/content.tsx (1)
5-5: LGTM! Import consolidation and type definitions look good.The changes properly address the previous duplicate constant issue by importing
ISSUE_RELATION_OPTIONSfrom a centralized location. The newTRelationObjecttype adds clarity to the relation object structure.Also applies to: 12-14, 27-33
web/core/components/issues/issue-detail/relation-select.tsx (2)
7-19: LGTM! Well-organized imports and type definitions.The reorganization of imports and centralization of
ISSUE_RELATION_OPTIONSimproves code maintainability and follows the DRY principle.
Line range hint
1-152: Verify the relationship with Timeline Layout revamp.This component handles issue relations but seems tangential to the PR's main objective of revamping Timeline Layout with Week, Month, and Quarter views. Could you clarify how these changes contribute to the Timeline Layout revamp?
Let's check for any Timeline-related connections:
web/core/store/issue/issue-details/relation.store.ts (1)
88-97: Effective Use ofcomputedFnfor MemoizationGreat use of
computedFnfrommobx-utilsto memoize thegetRelationCountByIssueIdmethod. This enhances performance by caching computed values based on inputs.
| // Plane-web | ||
| import { ISSUE_RELATION_OPTIONS } from "@/plane-web/components/relations"; | ||
| import { TIssueRelationTypes } from "@/plane-web/types"; |
There was a problem hiding this comment.
💡 Codebase verification
Import paths need attention - inconsistent usage detected
The search results reveal inconsistencies in the import paths:
- Most files use the new
@/plane-web/typespath forTIssueRelationTypes - However, files in
web/ce/still use relative paths (e.g.,"../../types","../types") - The
plane-webdirectory doesn't appear to exist in the filesystem (emptyfdresults)
This suggests:
- The import paths in the reviewed file are incorrect or premature
- The codebase is in an inconsistent state regarding these imports
- The migration to
@/plane-web/structure might be incomplete or incorrect
🔗 Analysis chain
Verify the consistency of new import paths across the codebase.
The migration of types and constants to centralized locations (@/plane-web/types and @/plane-web/components/relations) is a good practice for maintainability. Let's ensure this pattern is consistently followed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that old import paths are not still in use
# and new paths are consistently used across the codebase
# Check for any remaining old-style imports
echo "Checking for old import paths..."
rg "from ['\"]@plane/types['\"].*TIssueRelationTypes"
rg "from ['\"]./helper['\"].*ISSUE_RELATION_OPTIONS"
# Verify new import pattern usage
echo "Checking new import paths usage..."
rg "from ['\"]@/plane-web/types['\"].*TIssueRelationTypes"
rg "from ['\"]@/plane-web/components/relations['\"].*ISSUE_RELATION_OPTIONS"
Length of output: 420
Script:
#!/bin/bash
# Let's try a broader search to understand the usage patterns
# Search for any imports of TIssueRelationTypes
echo "Searching for TIssueRelationTypes imports..."
rg "import.*TIssueRelationTypes" -A 1
# Search for any imports of ISSUE_RELATION_OPTIONS
echo "Searching for ISSUE_RELATION_OPTIONS imports..."
rg "import.*ISSUE_RELATION_OPTIONS" -A 1
# Search for files containing these identifiers
echo "Searching for files containing these identifiers..."
rg "TIssueRelationTypes|ISSUE_RELATION_OPTIONS" --files-with-matches
# Check if the new directories exist
echo "Checking directory structure..."
fd "plane-web"
Length of output: 5150
| <div className="relative border-b border-custom-border-200"> | ||
| <div className="sticky left-0 inline-flex whitespace-nowrap px-2 py-1 text-sm font-medium capitalize"> | ||
| {_itemRoot?.title} | ||
| <div className={`absolute top-0 left-0 h-max w-max flex`} style={{ minHeight: `calc(100% + ${HEADER_HEIGHT}px` }}> |
There was a problem hiding this comment.
Fix incomplete style calculation.
The minHeight style calculation is missing a closing parenthesis.
- style={{ minHeight: `calc(100% + ${HEADER_HEIGHT}px` }}
+ style={{ minHeight: `calc(100% + ${HEADER_HEIGHT}px)` }}📝 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.
| <div className={`absolute top-0 left-0 h-max w-max flex`} style={{ minHeight: `calc(100% + ${HEADER_HEIGHT}px` }}> | |
| <div className={`absolute top-0 left-0 h-max w-max flex`} style={{ minHeight: `calc(100% + ${HEADER_HEIGHT}px)` }}> |
| {["sat", "sun"].includes(weekDay?.dayData?.shortTitle) && ( | ||
| <div className="h-full bg-custom-background-90 outline-[0.25px] outline outline-custom-border-300" /> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract weekend days to constants.
The weekend days check uses a hardcoded array literal. Consider moving this to a constant for better maintainability and reusability.
+ const WEEKEND_DAYS = ['sat', 'sun'] as const;
// Later in the code
- {["sat", "sun"].includes(weekDay?.dayData?.shortTitle) && (
+ {WEEKEND_DAYS.includes(weekDay?.dayData?.shortTitle) && (Committable suggestion was skipped due to low confidence.
| <div className={`absolute top-0 left-0 h-max w-max flex`} style={{ minHeight: `calc(100% + ${HEADER_HEIGHT}px` }}> | ||
| {currentViewData && |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving inline styles to CSS classes.
The component uses multiple inline styles for dimensions and positioning. Consider moving these to CSS classes for better maintainability and reusability.
// Create a new CSS module file: quarter-chart.module.css
+.timelineContainer {
+ min-height: calc(100% + var(--header-height));
+ position: absolute;
+ top: 0;
+ left: 0;
+ height: max-content;
+ width: max-content;
+ display: flex;
+}
+.headerHeight {
+ height: var(--header-height);
+}
+.sidebarOffset {
+ left: var(--sidebar-width);
+}
// Update the component
-<div className={`absolute top-0 left-0 h-max w-max flex`}
- style={{ minHeight: `calc(100% + ${HEADER_HEIGHT}px` }}>
+<div className={styles.timelineContainer}>Also applies to: 29-30, 37-38, 62-62, 85-85
| {quarterBlock?.children?.map((monthBlock, index) => ( | ||
| <div | ||
| key={`sub-title-${rootIndex}-${index}`} | ||
| className={cn( | ||
| "flex flex-shrink-0 text-center capitalize justify-center outline-[0.25px] outline outline-custom-border-200", | ||
| { | ||
| "bg-custom-primary-10": monthBlock.today, | ||
| } | ||
| )} | ||
| style={{ width: `${currentViewData?.data.dayWidth * monthBlock.days}px` }} | ||
| > | ||
| <div className="space-x-1 flex items-center justify-center text-xs font-medium h-full"> | ||
| <span | ||
| className={cn({ | ||
| "rounded-lg bg-custom-primary-100 px-2 text-white": monthBlock.today, | ||
| })} | ||
| > | ||
| <div>{_item.title}</div> | ||
| </div> | ||
| <div className={`relative flex h-full w-full flex-1 justify-center`}> | ||
| {_item?.today && <div className="absolute bottom-0 top-0 border border-red-500"> </div>} | ||
| </div> | ||
| {monthBlock.monthData.shortTitle} | ||
| </span> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize rendering performance with memoization.
The nested mapping of quarterBlock.children could benefit from memoization to prevent unnecessary re-renders.
+const MonthColumn = memo(({
+ monthBlock,
+ rootIndex,
+ index,
+ dayWidth
+}: MonthColumnProps) => (
+ <div
+ key={`column-${rootIndex}-${index}`}
+ className={cn("h-full overflow-hidden outline-[0.25px] outline outline-custom-border-100", {
+ "bg-custom-primary-10": monthBlock.today,
+ })}
+ style={{ width: `${dayWidth * monthBlock.days}px` }}
+ />
+));
+const MonthHeader = memo(({
+ monthBlock,
+ rootIndex,
+ index,
+ dayWidth
+}: MonthHeaderProps) => (
+ // ... existing month header JSX
+));Also applies to: 79-87
| updateBlockPosition = action((id: string, deltaLeft: number, deltaWidth: number) => { | ||
| const currBlock = this.blocksMap[id]; | ||
|
|
||
| if (!currBlock?.position) return; | ||
|
|
||
| const newMarginLeft = currBlock.position.marginLeft + deltaLeft; | ||
| const newWidth = currBlock.position.width + deltaWidth; | ||
|
|
||
| runInAction(() => { | ||
| set(this.blocksMap, [id, "position"], { | ||
| marginLeft: newMarginLeft ?? currBlock.position?.marginLeft, | ||
| width: newWidth ?? currBlock.position?.width, | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Ensure block dimensions remain valid after position updates
In the updateBlockPosition method, applying deltaWidth can result in a negative newWidth, which may cause rendering issues or errors in the Gantt chart.
Add validation to prevent newWidth from becoming negative:
const newWidth = currBlock.position.width + deltaWidth;
+ if (newWidth < 0) {
+ // Prevent negative width by setting it to a minimum value or skipping the update
+ return;
+ }This ensures that the block's width remains logical and the UI remains stable.
Committable suggestion was skipped due to low confidence.
| // actions | ||
| fetchRelations: action, | ||
| createRelation: action, | ||
| createCurrentRelation: action, |
There was a problem hiding this comment.
Include New Actions in makeObservable
The methods createCurrentRelation and extractRelationsFromIssues should be included in the makeObservable configuration to ensure they are properly observed by MobX.
Apply this diff to include the new methods:
makeObservable(this, {
// observables
relationMap: observable,
// computed
issueRelations: computed,
// actions
fetchRelations: action,
createRelation: action,
+ createCurrentRelation: action,
removeRelation: action,
+ extractRelationsFromIssues: action,
});Also applies to: 67-67
| const reverseRelatedType = REVERSE_RELATIONS[relationType]; | ||
|
|
||
| const issuesOfRelation = get(this.relationMap, [issueId, relationType]) ?? []; | ||
|
|
||
| if (response && response.length > 0) | ||
| runInAction(() => { | ||
| response.forEach((issue) => { | ||
| const issuesOfRelated = get(this.relationMap, [issue.id, reverseRelatedType]); | ||
| this.rootIssueDetailStore.rootIssueStore.issues.addIssue([issue]); | ||
| this.relationMap[issueId][relationType].push(issue.id); | ||
| issuesOfRelation.push(issue.id); | ||
|
|
||
| if (!issuesOfRelated) { | ||
| set(this.relationMap, [issue.id, reverseRelatedType], [issueId]); | ||
| } else { | ||
| set(this.relationMap, [issue.id, reverseRelatedType], uniq([...issuesOfRelated, issueId])); | ||
| } | ||
| }); | ||
| set(this.relationMap, [issueId, relationType], uniq(issuesOfRelation)); |
There was a problem hiding this comment.
Handle Undefined Values Safely in createRelation
When accessing properties like issuesOfRelated, ensure that you handle cases where these may be undefined to prevent runtime errors.
Consider using optional chaining or default values to safeguard against undefined properties.
|
|
||
| /** | ||
| * Extract Relation from the issue Array objects and store it in this Store | ||
| * @param issues | ||
| */ | ||
| extractRelationsFromIssues = (issues: TIssue[]) => { | ||
| try { | ||
| runInAction(() => { | ||
| for (const issue of issues) { | ||
| const { issue_relation, issue_related, id: issueId } = issue; | ||
|
|
||
| const issueRelations: { [key in TIssueRelationTypes]?: string[] } = {}; | ||
|
|
||
| if (issue_relation && Array.isArray(issue_relation) && issue_relation.length) { | ||
| for (const relation of issue_relation) { | ||
| const { relation_type, id } = relation; | ||
|
|
||
| if (!relation_type) continue; | ||
|
|
||
| if (issueRelations[relation_type]) issueRelations[relation_type]?.push(id); | ||
| else issueRelations[relation_type] = [id]; | ||
| } | ||
| } | ||
|
|
||
| if (issue_related && Array.isArray(issue_related) && issue_related.length) { | ||
| for (const relation of issue_related) { | ||
| const { relation_type, id } = relation; | ||
|
|
||
| if (!relation_type) continue; | ||
|
|
||
| const reverseRelatedType = REVERSE_RELATIONS[relation_type as TIssueRelationTypes]; | ||
|
|
||
| if (issueRelations[reverseRelatedType]) issueRelations[reverseRelatedType]?.push(id); | ||
| else issueRelations[reverseRelatedType] = [id]; | ||
| } | ||
| } | ||
|
|
||
| set(this.relationMap, [issueId], issueRelations); | ||
| } | ||
| }); | ||
| } catch (e) { | ||
| console.error("Error while extracting issue relations from issues"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize extractRelationsFromIssues Method
Consider optimizing the loops within the extractRelationsFromIssues method. Using more efficient looping constructs or array methods can enhance performance, especially with large data sets.
For example, you might replace the for...of loops with forEach or use functional programming methods like map and reduce where appropriate.
| const relationIndex = this.relationMap[issueId]?.[relationType]?.findIndex( | ||
| (_issueId) => _issueId === related_issue | ||
| ); | ||
| if (relationIndex >= 0) | ||
| runInAction(() => { | ||
| this.relationMap[issueId][relationType].splice(relationIndex, 1); | ||
| this.relationMap[issueId]?.[relationType]?.splice(relationIndex, 1); | ||
| }); | ||
|
|
||
| const response = await this.issueRelationService.deleteIssueRelation(workspaceSlug, projectId, issueId, { | ||
| relation_type: relationType, | ||
| related_issue, | ||
| }); | ||
|
|
||
| // While removing one relation, reverse of the relation should also be removed | ||
| const reverseRelatedType = REVERSE_RELATIONS[relationType]; | ||
| const relatedIndex = this.relationMap[related_issue]?.[reverseRelatedType]?.findIndex( | ||
| (_issueId) => _issueId === related_issue | ||
| ); | ||
| if (relationIndex >= 0) | ||
| runInAction(() => { | ||
| this.relationMap[related_issue]?.[reverseRelatedType]?.splice(relatedIndex, 1); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Correct Index Reference When Removing Reverse Relations
There's a potential mismatch in index variables when removing the reverse relation. Ensure that you're using relatedIndex when splicing the reverse relation array.
Apply this fix:
if (relationIndex >= 0)
runInAction(() => {
- this.relationMap[related_issue]?.[reverseRelatedType]?.splice(relationIndex, 1);
+ this.relationMap[related_issue]?.[reverseRelatedType]?.splice(relatedIndex, 1);
});📝 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 relationIndex = this.relationMap[issueId]?.[relationType]?.findIndex( | |
| (_issueId) => _issueId === related_issue | |
| ); | |
| if (relationIndex >= 0) | |
| runInAction(() => { | |
| this.relationMap[issueId][relationType].splice(relationIndex, 1); | |
| this.relationMap[issueId]?.[relationType]?.splice(relationIndex, 1); | |
| }); | |
| const response = await this.issueRelationService.deleteIssueRelation(workspaceSlug, projectId, issueId, { | |
| relation_type: relationType, | |
| related_issue, | |
| }); | |
| // While removing one relation, reverse of the relation should also be removed | |
| const reverseRelatedType = REVERSE_RELATIONS[relationType]; | |
| const relatedIndex = this.relationMap[related_issue]?.[reverseRelatedType]?.findIndex( | |
| (_issueId) => _issueId === related_issue | |
| ); | |
| if (relationIndex >= 0) | |
| runInAction(() => { | |
| this.relationMap[related_issue]?.[reverseRelatedType]?.splice(relatedIndex, 1); | |
| }); | |
| const relationIndex = this.relationMap[issueId]?.[relationType]?.findIndex( | |
| (_issueId) => _issueId === related_issue | |
| ); | |
| if (relationIndex >= 0) | |
| runInAction(() => { | |
| this.relationMap[issueId]?.[relationType]?.splice(relationIndex, 1); | |
| }); | |
| const response = await this.issueRelationService.deleteIssueRelation(workspaceSlug, projectId, issueId, { | |
| relation_type: relationType, | |
| related_issue, | |
| }); | |
| // While removing one relation, reverse of the relation should also be removed | |
| const reverseRelatedType = REVERSE_RELATIONS[relationType]; | |
| const relatedIndex = this.relationMap[related_issue]?.[reverseRelatedType]?.findIndex( | |
| (_issueId) => _issueId === related_issue | |
| ); | |
| if (relationIndex >= 0) | |
| runInAction(() => { | |
| this.relationMap[related_issue]?.[reverseRelatedType]?.splice(relatedIndex, 1); | |
| }); |
3 participants
This PR Revamps the Timeline layout to have three views, instead of previously available month view. These also include other minor improvements in performance and UX
The new Timeline Views are as follows,
Week View
Screen.Recording.2024-10-25.at.7.57.37.PM.mov
Month View
Screen.Recording.2024-10-25.at.7.58.30.PM.mov
Quarter View
Screen.Recording.2024-10-25.at.7.59.26.PM.mov
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores