Skip to content

[WEB-2442] chore: Refactor timeline store #5926

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pushya22 merged 1 commit into from
Oct 29, 2024
Merged

[WEB-2442] chore: Refactor timeline store #5926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions web/ce/store/issue/issue-details/activity.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ import { computedFn } from "mobx-utils";
import { TIssueActivityComment, TIssueActivity, TIssueActivityMap, TIssueActivityIdMap } from "@plane/types";
// plane web constants
import { EActivityFilterType } from "@/plane-web/constants/issues";
// plane web store types
import { RootStore } from "@/plane-web/store/root.store";
// services
import { IssueActivityService } from "@/services/issue";
// store
import { CoreRootStore } from "@/store/root.store";

export type TActivityLoader = "fetch" | "mutate" | undefined;

Expand Down Expand Up @@ -47,7 +47,7 @@ export class IssueActivityStore implements IIssueActivityStore {
// services
issueActivityService;

constructor(protected store: RootStore) {
constructor(protected store: CoreRootStore) {
makeObservable(this, {
// observables
loader: observable.ref,
Expand Down
5 changes: 5 additions & 0 deletions web/ce/store/root.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
// store
import { CoreRootStore } from "@/store/root.store";
import { ITimelineStore, TimeLineStore } from "./timeline";

export class RootStore extends CoreRootStore {
timelineStore: ITimelineStore;

constructor() {
super();

this.timelineStore = new TimeLineStore(this);
}
}
7 changes: 5 additions & 2 deletions web/ce/store/timeline/base-timeline.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { RootStore } from "@/plane-web/store/root.store";
type BlockData = {
id: string;
name: string;
sort_order: number;
sort_order: number | null;
start_date: string | undefined | null;
target_date: string | undefined | null;
};
Expand All @@ -27,6 +27,7 @@ export interface IBaseTimelineStore {
activeBlockId: string | null;
renderView: any;
isDragging: boolean;
isDependencyEnabled: boolean;
//
setBlockIds: (ids: string[]) => void;
getBlockById: (blockId: string) => IGanttBlock;
Expand Down Expand Up @@ -60,6 +61,8 @@ export class BaseTimeLineStore implements IBaseTimelineStore {

rootStore: RootStore;

isDependencyEnabled = false;

Comment on lines +64 to +65
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add observable decorator for isDependencyEnabled.

The isDependencyEnabled property should be marked as observable since it's part of the store's state and might trigger UI updates when changed.

Apply this change to the makeObservable call:

 makeObservable(this, {
   // observables
   blocksMap: observable,
   blockIds: observable,
   isDragging: observable.ref,
   currentView: observable.ref,
   currentViewData: observable,
   activeBlockId: observable.ref,
   renderView: observable,
+  isDependencyEnabled: observable,
   // actions
   setIsDragging: action,
   setBlockIds: action.bound,

Also applies to: 67-85

constructor(_rootStore: RootStore) {
makeObservable(this, {
// observables
Expand Down Expand Up @@ -179,7 +182,7 @@ export class BaseTimeLineStore implements IBaseTimelineStore {
data: blockData,
id: blockData?.id,
name: blockData.name,
sort_order: blockData?.sort_order,
sort_order: blockData?.sort_order ?? undefined,
start_date: blockData?.start_date ?? undefined,
target_date: blockData?.target_date ?? undefined,
};
Expand Down
23 changes: 23 additions & 0 deletions web/ce/store/timeline/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { RootStore } from "@/plane-web/store/root.store";
import { IIssuesTimeLineStore, IssuesTimeLineStore } from "@/store/timeline/issues-timeline.store";
import { IModulesTimeLineStore, ModulesTimeLineStore } from "@/store/timeline/modules-timeline.store";
import { BaseTimeLineStore, IBaseTimelineStore } from "./base-timeline.store";

export interface ITimelineStore {
issuesTimeLineStore: IIssuesTimeLineStore;
modulesTimeLineStore: IModulesTimeLineStore;
projectTimeLineStore: IBaseTimelineStore;
}

export class TimeLineStore implements ITimelineStore {
issuesTimeLineStore: IIssuesTimeLineStore;
modulesTimeLineStore: IModulesTimeLineStore;
projectTimeLineStore: IBaseTimelineStore;

constructor(rootStore: RootStore) {
this.issuesTimeLineStore = new IssuesTimeLineStore(rootStore);
this.modulesTimeLineStore = new ModulesTimeLineStore(rootStore);
// Dummy store
this.projectTimeLineStore = new BaseTimeLineStore(rootStore);
}
Comment on lines +17 to +22
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding initialization checks.

The constructor instantiates stores without any error handling. Consider adding checks to ensure rootStore is properly initialized and contains required dependencies.

 constructor(rootStore: RootStore) {
+  if (!rootStore) {
+    throw new Error("RootStore is required for TimeLineStore initialization");
+  }
   this.issuesTimeLineStore = new IssuesTimeLineStore(rootStore);
   this.modulesTimeLineStore = new ModulesTimeLineStore(rootStore);
   // Dummy store
   this.projectTimeLineStore = new BaseTimeLineStore(rootStore);
 }
📝 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.

Suggested change
constructor(rootStore: RootStore) {
this.issuesTimeLineStore = new IssuesTimeLineStore(rootStore);
this.modulesTimeLineStore = new ModulesTimeLineStore(rootStore);
// Dummy store
this.projectTimeLineStore = new BaseTimeLineStore(rootStore);
}
constructor(rootStore: RootStore) {
if (!rootStore) {
throw new Error("RootStore is required for TimeLineStore initialization");
}
this.issuesTimeLineStore = new IssuesTimeLineStore(rootStore);
this.modulesTimeLineStore = new ModulesTimeLineStore(rootStore);
// Dummy store
this.projectTimeLineStore = new BaseTimeLineStore(rootStore);
}

}
1 change: 1 addition & 0 deletions web/core/components/gantt-chart/contexts/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { createContext, useContext } from "react";
export enum ETimeLineTypeType {
ISSUE = "ISSUE",
MODULE = "MODULE",
PROJECT = "PROJECT",
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Timeline store integration needs attention

The verification revealed that while the PROJECT enum is being used, its implementation is incomplete:

  • The projectTimeLineStore is currently using a dummy implementation: new BaseTimeLineStore(rootStore) with a comment "// Dummy store"
  • Other timeline types (ISSUE, MODULE) have dedicated store implementations (IssuesTimeLineStore, ModulesTimeLineStore), but PROJECT uses the base class

This suggests the PROJECT timeline functionality is not fully implemented yet.

Action items:

  • Implement a dedicated ProjectTimeLineStore class extending TimelineStore
  • Replace the dummy store with the proper implementation
  • Ensure all PROJECT timeline-specific functionality is implemented in the new store
🔗 Analysis chain

LGTM! Verify timeline store integration.

The addition of PROJECT to the enum follows the existing pattern and maintains consistency. The change aligns well with the timeline store refactoring objective.

Let's verify the integration with timeline stores:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that PROJECT timeline type is properly integrated with timeline stores
# and that there are no missing implementations.

# Test 1: Check for PROJECT timeline store implementation
ast-grep --pattern 'projectTimeLineStore'

# Test 2: Check for PROJECT case handling in timeline-related code
rg -l "PROJECT.*timeline" 

# Test 3: Look for any TODO or unimplemented cases
rg "TODO.*PROJECT.*timeline"

Length of output: 100

Script:

#!/bin/bash
# Let's try a more comprehensive search strategy

# Check for PROJECT enum usage in timeline-related files
rg -i "PROJECT.*timeline|timeline.*PROJECT" -A 3 -B 3

# Look for timeline store implementations
ast-grep --pattern 'class $_ extends TimelineStore'

# Check for switch/case statements handling timeline types
ast-grep --pattern 'switch ($timelineType) {
  $$$
}'

# Search for timeline type handling in general
rg "ETimeLineTypeType" -A 2 -B 2

# Look for store factory or initialization code
ast-grep --pattern 'new $_TimelineStore'

Length of output: 9402

}

export const TimeLineTypeContext = createContext<ETimeLineTypeType | undefined>(undefined);
Expand Down
10 changes: 5 additions & 5 deletions web/core/components/gantt-chart/sidebar/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,17 @@ export const handleOrderChange = (
// return if dropped outside the list
if (sourceBlockIndex === -1 || destinationBlockIndex === -1 || sourceBlockIndex === destinationBlockIndex) return;

let updatedSortOrder = getBlockById(blockIds[sourceBlockIndex])?.sort_order;
let updatedSortOrder = getBlockById(blockIds[sourceBlockIndex])?.sort_order ?? 0;

// update the sort order to the lowest if dropped at the top
if (destinationBlockIndex === 0) updatedSortOrder = getBlockById(blockIds[0])?.sort_order - 1000;
if (destinationBlockIndex === 0) updatedSortOrder = (getBlockById(blockIds[0])?.sort_order ?? 0) - 1000;
// update the sort order to the highest if dropped at the bottom
else if (destinationBlockIndex === blockIds.length)
updatedSortOrder = getBlockById(blockIds[blockIds.length - 1])?.sort_order + 1000;
updatedSortOrder = (getBlockById(blockIds[blockIds.length - 1])?.sort_order ?? 0) + 1000;
// update the sort order to the average of the two adjacent blocks if dropped in between
else {
const destinationSortingOrder = getBlockById(blockIds[destinationBlockIndex])?.sort_order;
const relativeDestinationSortingOrder = getBlockById(blockIds[destinationBlockIndex - 1])?.sort_order;
const destinationSortingOrder = getBlockById(blockIds[destinationBlockIndex])?.sort_order ?? 0;
const relativeDestinationSortingOrder = getBlockById(blockIds[destinationBlockIndex - 1])?.sort_order ?? 0;

updatedSortOrder = (destinationSortingOrder + relativeDestinationSortingOrder) / 2;
}
Expand Down
2 changes: 1 addition & 1 deletion web/core/components/gantt-chart/types/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export interface IGanttBlock {
marginLeft: number;
width: number;
};
sort_order: number;
sort_order: number | undefined;
start_date: string | undefined;
target_date: string | undefined;
}
Expand Down
11 changes: 7 additions & 4 deletions web/core/hooks/use-timeline-chart.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { useContext } from "react";
// mobx store
// types
// lib
import { StoreContext } from "@/lib/store-context";
import { IBaseTimelineStore } from "ee/store/timeline/base-timeline.store";
// Plane-web
import { IBaseTimelineStore } from "@/plane-web/store/timeline/base-timeline.store";
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Import path change requires attention

The import path @/plane-web/store/timeline/base-timeline.store in the review file is incorrect. The search results show that:

  1. The base-timeline.store.ts exists in two locations:

    • web/ce/store/timeline/base-timeline.store.ts
    • web/ee/store/timeline/base-timeline.store.ts

  2. Current imports in the codebase use:

    • "ce/store/timeline/base-timeline.store"
    • "./base-timeline.store"
    • "ce/store/timeline/base-timeline.store"

The import path should be updated to use the correct path "ce/store/timeline/base-timeline.store" to maintain consistency with other files in the codebase.

🔗 Analysis chain

Verify the import path change impact.

The import path change from 'ee/store' to '@/plane-web/store' appears to be part of the code splitting objective. Let's verify that this change is consistent across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining references to the old import path
# and verify the new path exists

# Test 1: Check for any remaining references to the old path
echo "Checking for old import path references..."
rg "from ['\"]ee/store/timeline/base-timeline\.store['\"]"

# Test 2: Verify the new path exists
echo "Verifying new path exists..."
fd "base-timeline.store.ts" "web/plane-web/store/timeline"

Length of output: 399

Script:

#!/bin/bash
# Let's try a broader search approach

# Test 1: Find the base-timeline.store.ts file anywhere in the repo
echo "Searching for base-timeline.store.ts..."
fd "base-timeline.store.ts"

# Test 2: Check for any imports of base-timeline.store from any path
echo "Checking all imports of base-timeline.store..."
rg "from.*base-timeline\.store"

# Test 3: List all files in web directory to understand structure
echo "Listing web directory structure..."
fd . web -t d

Length of output: 21793

//
import { ETimeLineTypeType, useTimeLineType } from "../components/gantt-chart/contexts";

export const useTimeLineChart = (timeLineType: ETimeLineTypeType): IBaseTimelineStore => {
Expand All @@ -13,7 +14,9 @@ export const useTimeLineChart = (timeLineType: ETimeLineTypeType): IBaseTimeline
case ETimeLineTypeType.ISSUE:
return context.timelineStore.issuesTimeLineStore;
case ETimeLineTypeType.MODULE:
return context.timelineStore.modulesTimeLineStore;
return context.timelineStore.modulesTimeLineStore as IBaseTimelineStore;
case ETimeLineTypeType.PROJECT:
return context.timelineStore.projectTimeLineStore as IBaseTimelineStore;
}
};

Expand Down
9 changes: 4 additions & 5 deletions web/core/store/issue/root.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import isEmpty from "lodash/isEmpty";
import { autorun, makeObservable, observable } from "mobx";
import { ICycle, IIssueLabel, IModule, IProject, IState, IUserLite } from "@plane/types";
// plane web root store
import { RootStore } from "@/plane-web/store/root.store";
// root store
import { IWorkspaceMembership } from "@/store/member/workspace-member.store";
import { CoreRootStore } from "../root.store";
import { IStateStore, StateStore } from "../state.store";
// issues data store
import { IArchivedIssuesFilter, ArchivedIssuesFilter, IArchivedIssues, ArchivedIssues } from "./archived";
Expand Down Expand Up @@ -50,7 +49,7 @@ export interface IIssueRootStore {
moduleMap: Record<string, IModule> | undefined;
cycleMap: Record<string, ICycle> | undefined;

rootStore: RootStore;
rootStore: CoreRootStore;

issues: IIssueStore;

Expand Down Expand Up @@ -108,7 +107,7 @@ export class IssueRootStore implements IIssueRootStore {
moduleMap: Record<string, IModule> | undefined = undefined;
cycleMap: Record<string, ICycle> | undefined = undefined;

rootStore: RootStore;
rootStore: CoreRootStore;

issues: IIssueStore;

Expand Down Expand Up @@ -146,7 +145,7 @@ export class IssueRootStore implements IIssueRootStore {
issueKanBanView: IIssueKanBanViewStore;
issueCalendarView: ICalendarStore;

constructor(rootStore: RootStore) {
constructor(rootStore: CoreRootStore) {
makeObservable(this, {
workspaceSlug: observable.ref,
projectId: observable.ref,
Expand Down
3 changes: 0 additions & 3 deletions web/core/store/root.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import { IProjectViewStore, ProjectViewStore } from "./project-view.store";
import { RouterStore, IRouterStore } from "./router.store";
import { IStateStore, StateStore } from "./state.store";
import { ThemeStore, IThemeStore } from "./theme.store";
import { ITimelineStore, TimeLineStore } from "./timeline";
import { ITransientStore, TransientStore } from "./transient.store";
import { IUserStore, UserStore } from "./user";
import { IWorkspaceRootStore, WorkspaceRootStore } from "./workspace";
Expand Down Expand Up @@ -57,7 +56,6 @@ export class CoreRootStore {
workspaceNotification: IWorkspaceNotificationStore;
favorite: IFavoriteStore;
transient: ITransientStore;
timelineStore: ITimelineStore;

constructor() {
this.router = new RouterStore();
Expand Down Expand Up @@ -86,7 +84,6 @@ export class CoreRootStore {
this.workspaceNotification = new WorkspaceNotificationStore(this);
this.favorite = new FavoriteStore(this);
this.transient = new TransientStore();
this.timelineStore = new TimeLineStore(this);
}

resetOnSignOut() {
Expand Down
19 changes: 0 additions & 19 deletions web/core/store/timeline/index.ts
View file
Open in desktop

This file was deleted.

5 changes: 1 addition & 4 deletions web/core/store/timeline/issues-timeline.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,10 @@ export interface IIssuesTimeLineStore extends IBaseTimelineStore {
}

export class IssuesTimeLineStore extends BaseTimeLineStore implements IIssuesTimeLineStore {
isDependencyEnabled = true;

constructor(_rootStore: RootStore) {
super(_rootStore);

autorun((reaction) => {
reaction.trace();
autorun(() => {
const getIssueById = this.rootStore.issue.issues.getIssueById;
this.updateBlocks(getIssueById);
});
Expand Down
5 changes: 1 addition & 4 deletions web/core/store/timeline/modules-timeline.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,10 @@ export interface IModulesTimeLineStore extends IBaseTimelineStore {
}

export class ModulesTimeLineStore extends BaseTimeLineStore implements IModulesTimeLineStore {
isDependencyEnabled = false;

constructor(_rootStore: RootStore) {
super(_rootStore);

autorun((reaction) => {
reaction.trace();
autorun(() => {
const getModuleById = this.rootStore.module.getModuleById;
this.updateBlocks(getModuleById);
});
Expand Down