[WEB-2442] chore: Refactor timeline store#5926
Conversation
WalkthroughThe changes in this pull request involve modifications to various store classes and interfaces within the application. Key updates include changing the constructor parameters from Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (11)
web/ce/store/root.store.ts (1)
1-12: Clean architectural separation of timeline functionality.
The refactoring moves timeline management from CoreRootStore to RootStore, which aligns well with the goal of effective code splitting. This separation of concerns allows for better modularity and maintainability.
This change:
- Keeps core functionality lean in CoreRootStore
- Allows timeline features to be managed independently in the CE version
- Facilitates easier testing and maintenance
web/core/store/timeline/modules-timeline.store.ts (2)
Line range hint 6-8: Fix interface-implementation mismatch.
The IModulesTimeLineStore interface requires an isDependencyEnabled property, but the implementing class ModulesTimeLineStore doesn't provide it. This violates the TypeScript contract and could lead to runtime issues.
Consider one of these solutions:
- export interface IModulesTimeLineStore extends IBaseTimelineStore {
- isDependencyEnabled: boolean;
- }
+ export interface IModulesTimeLineStore extends IBaseTimelineStore {}Or implement the property in the class:
export class ModulesTimeLineStore extends BaseTimeLineStore implements IModulesTimeLineStore {
+ isDependencyEnabled: boolean = false;
constructor(_rootStore: RootStore) {14-17: Consider preserving debugging capabilities in autorun.
The simplified autorun implementation removes the previous reaction.trace() capability, which could make debugging more difficult in development.
Consider preserving the debugging capability in development:
- autorun(() => {
+ autorun((reaction) => {
const getModuleById = this.rootStore.module.getModuleById;
this.updateBlocks(getModuleById);
+ if (process.env.NODE_ENV === 'development') {
+ reaction.trace();
+ }
});web/core/store/timeline/issues-timeline.store.ts (1)
Line range hint 6-8: Fix interface-implementation mismatch.
The interface IIssuesTimeLineStore requires isDependencyEnabled property, but the implementation IssuesTimeLineStore doesn't provide it. This will cause TypeScript compilation errors.
Either remove the property from the interface or implement it in the class:
export interface IIssuesTimeLineStore extends IBaseTimelineStore {
- isDependencyEnabled: boolean;
}web/ce/store/timeline/index.ts (1)
1-4: Consider standardizing import paths.
The imports mix different path styles (@/plane-web/store, @/store, and relative paths). Consider standardizing them for better maintainability.
web/core/hooks/use-timeline-chart.ts (2)
16-19: Maintain consistent type casting across all cases.
The MODULE and PROJECT cases use explicit type casting to IBaseTimelineStore, while the ISSUE case doesn't. For consistency and type safety, consider applying the same type casting pattern to all cases.
switch (timeLineType) {
case ETimeLineTypeType.ISSUE:
- return context.timelineStore.issuesTimeLineStore;
+ return context.timelineStore.issuesTimeLineStore as IBaseTimelineStore;
case ETimeLineTypeType.MODULE:
return context.timelineStore.modulesTimeLineStore as IBaseTimelineStore;
case ETimeLineTypeType.PROJECT:
return context.timelineStore.projectTimeLineStore as IBaseTimelineStore;
}Line range hint 9-21: Consider adding JSDoc documentation for the timeline types.
The useTimeLineChart hook handles multiple timeline types (ISSUE, MODULE, PROJECT) but lacks documentation explaining the differences and use cases for each type. Adding JSDoc comments would improve maintainability and developer experience.
+/**
+ * Hook to access the appropriate timeline store based on the timeline type.
+ * @param timeLineType - The type of timeline to access:
+ * - ISSUE: For issue-based timelines
+ * - MODULE: For module-based timelines
+ * - PROJECT: For project-based timelines
+ * @returns The corresponding timeline store instance
+ * @throws {Error} If used outside of StoreProvider
+ */
export const useTimeLineChart = (timeLineType: ETimeLineTypeType): IBaseTimelineStore => {web/core/components/gantt-chart/sidebar/utils.ts (2)
22-22: Consider using a configurable offset value.
While the current implementation works, the hardcoded value of 1000 for the offset might be better as a named constant or configuration parameter for maintainability.
+ const SORT_ORDER_OFFSET = 1000;
- updatedSortOrder = (getBlockById(blockIds[0])?.sort_order ?? 0) - 1000;
+ updatedSortOrder = (getBlockById(blockIds[0])?.sort_order ?? 0) - SORT_ORDER_OFFSET;19-31: Consider implementing sort order normalization.
The current approach of using division for middle insertions could lead to very small differences between sort orders after multiple insertions. Consider implementing a normalization function that redistributes sort orders when they get too close.
Would you like me to propose a sort order normalization implementation that:
- Detects when sort orders become too close
- Redistributes them evenly
- Maintains relative ordering
web/core/store/issue/root.store.ts (1)
Line range hint 1-224: Architecture feedback on store refactoring.
The transition from RootStore to CoreRootStore aligns well with the PR's objective of facilitating effective code splitting. This change suggests a move towards a more modular store architecture where core functionality is separated from feature-specific stores.
Consider documenting the architectural decision and migration strategy for other teams who might need to perform similar refactoring in their stores.
web/ce/store/timeline/base-timeline.store.ts (1)
Line range hint 179-194: Consider enhancing error handling in updateBlocks.
The block update logic could benefit from more robust error handling, especially for edge cases:
- Invalid block data validation
- Error reporting for failed updates
- Logging for debugging purposes
Consider wrapping the block creation in a try-catch and adding validation:
try {
const block: IGanttBlock = {
data: validateBlockData(blockData), // Add validation
id: blockData?.id,
name: blockData.name,
sort_order: blockData?.sort_order ?? undefined,
start_date: blockData?.start_date ?? undefined,
target_date: blockData?.target_date ?? undefined,
};
// ... rest of the code
} catch (error) {
console.error(`Failed to update block ${blockId}:`, error);
// Handle or report error appropriately
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- web/ce/store/issue/issue-details/activity.store.ts (2 hunks)
- web/ce/store/root.store.ts (1 hunks)
- web/ce/store/timeline/base-timeline.store.ts (4 hunks)
- web/ce/store/timeline/index.ts (1 hunks)
- web/core/components/gantt-chart/contexts/index.tsx (1 hunks)
- web/core/components/gantt-chart/sidebar/utils.ts (1 hunks)
- web/core/components/gantt-chart/types/index.ts (1 hunks)
- web/core/hooks/use-timeline-chart.ts (2 hunks)
- web/core/store/issue/root.store.ts (4 hunks)
- web/core/store/root.store.ts (0 hunks)
- web/core/store/timeline/index.ts (0 hunks)
- web/core/store/timeline/issues-timeline.store.ts (1 hunks)
- web/core/store/timeline/modules-timeline.store.ts (1 hunks)
💤 Files with no reviewable changes (2)
- web/core/store/root.store.ts
- web/core/store/timeline/index.ts
🔇 Additional comments (19)
web/ce/store/root.store.ts (1)
10-11: Verify timeline functionality after store initialization.
The timeline store is properly initialized with the parent store reference. However, we should ensure that any existing timeline features continue to work as expected after this architectural change.
✅ Verification successful
Let me gather more information about the timeline store implementation and its dependencies.
Let me check one more aspect to verify the timeline store's integration and dependencies.
Based on the gathered information, I can now provide the final response:
Timeline store initialization and integration is properly implemented
The verification shows that:
- TimeLineStore is correctly initialized with proper parent store reference
- All child stores (issues, modules, project) are properly instantiated
- The store is well-integrated with components through the
useTimelineCharthook - No circular dependencies were found in the implementation
The architectural change maintains the expected structure and dependencies, and the timeline functionality should continue to work as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify timeline store usage and dependencies
# Check for timeline store references in components and other stores
echo "Checking timeline store usage patterns:"
rg -A 3 "timelineStore" --type ts
# Look for potential circular dependencies
echo "Checking for potential circular dependencies:"
rg -l "import.*from.*timeline" --type ts | xargs rg "import.*from.*root.store"
Length of output: 1592
Script:
#!/bin/bash
# Check timeline store implementation and interfaces
echo "Checking timeline store implementation:"
ast-grep --pattern 'class TimeLineStore {
$$$
}'
echo "\nChecking timeline store interface:"
ast-grep --pattern 'interface ITimelineStore {
$$$
}'
echo "\nChecking timeline store initialization and usage in components:"
rg -A 5 "useTimelineChart" --type ts
echo "\nChecking timeline-related imports in the root store:"
rg "import.*Timeline" web/ce/store/root.store.ts
Length of output: 1028
Script:
#!/bin/bash
# Check timeline store's child stores and their initialization
echo "Checking timeline store's child stores:"
rg -A 10 "class TimeLineStore" --type ts
echo "\nChecking timeline store's usage in components:"
rg -B 2 -A 2 "useTimelineChart|timelineStore" web/core/components/ --type ts
echo "\nChecking timeline-related tests:"
fd -e test.ts -e spec.ts | xargs rg "timeline|Timeline" -l
Length of output: 3228
web/core/store/timeline/issues-timeline.store.ts (2)
14-17: LGTM! Simplified autorun logic.
The autorun callback has been simplified by removing the unused reaction parameter, making the code cleaner while maintaining the same functionality.
Line range hint 1-17: Verify the removal of isDependencyEnabled property.
The removal of isDependencyEnabled property might affect consumers of this class.
Let's verify its usage in the codebase:
web/core/components/gantt-chart/types/index.ts (1)
9-11: LGTM! Consider adding JSDoc comments.
The changes to make sort_order, start_date, and target_date optional align well with the timeline store refactoring objective. However, to improve maintainability:
- Consider adding JSDoc comments to document why these fields are optional and how they should be handled.
- Ensure all consumers of
IGanttBlockproperly handle the undefined cases.
Let's verify the impact on consumers:
✅ Verification successful
Based on the extensive search results, I can now generate the final response since I have enough context to verify the review comment.
Optional fields in IGanttBlock are properly handled throughout the codebase
The changes to make sort_order, start_date, and target_date optional in IGanttBlock interface are safe and align with the codebase's implementation:
- The base timeline store handles undefined values with nullish coalescing:
sort_order: blockData?.sort_order ?? undefined,
start_date: blockData?.start_date ?? undefined,
target_date: blockData?.target_date ?? undefined,- All consumers properly check for existence before using these fields:
- Gantt blocks:
if (!block || (!showAllBlocks && !(block.start_date && block.target_date))) - Position calculations:
const { start_date, target_date } = itemData; if (!itemStartDate || !itemTargetDate) return; - Sidebar components:
const isBlockVisibleOnChart = !!block?.start_date && !!block?.target_date;
The changes are part of a well-structured refactoring that maintains type safety while providing more flexibility in the timeline stores.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of IGanttBlock to ensure proper handling of optional fields
# Search for IGanttBlock usage
echo "Searching for IGanttBlock implementations and usage..."
rg -A 5 "IGanttBlock"
# Search for direct property access that might need null checks
echo "Checking for direct property access..."
rg -A 2 "(sort_order|start_date|target_date)\s*[^?]" web/
Length of output: 113764
web/ce/store/timeline/index.ts (3)
6-10: LGTM! Well-structured interface definition.
The interface is well-defined with proper typing for each timeline store component.
20-21: Clarify the purpose of the dummy store.
The comment indicates this is a "Dummy store". Please clarify:
- Is this a temporary implementation?
- What's the intended final implementation?
- Should this be tracked as a TODO item?
Would you like me to create a GitHub issue to track the implementation of the actual project timeline store?
17-22: Verify the RootStore type usage.
The AI summary mentions changes from RootStore to CoreRootStore, but this file still uses RootStore. Please verify if this is intentional.
web/core/hooks/use-timeline-chart.ts (1)
Line range hint 23-29: LGTM! Well-structured hook composition.
The useTimeLineChartStore hook demonstrates good practices:
- Proper error handling for missing context
- Clear composition of hooks
- Single responsibility principle
web/core/components/gantt-chart/sidebar/utils.ts (2)
19-19: LGTM: Safely handle undefined sort_order.
The addition of nullish coalescing operator ensures a default value of 0 when sort_order is undefined, preventing potential NaN or undefined arithmetic operations.
25-25: Verify sort order calculations for edge cases.
The current implementation assumes there's always enough space between sort orders for new items. We should verify this assumption and potentially implement a rebalancing mechanism.
Also applies to: 28-29
✅ Verification successful
Based on the search results, I can now provide a final response since I have enough context about the sort order implementation:
The sort order implementation has sufficient spacing and rebalancing mechanisms in place.
The codebase shows a well-designed sort order system with:
- Default spacing of 1000 units between items when reordering at top/bottom (
+/- 1000) - Average-based positioning for items dropped between two existing items (
(destinationSortingOrder + relativeDestinationSortingOrder) / 2) - Large default value of 65535 for initial items
- Backend support for rebalancing with 10000-unit spacing when needed
The implementation follows best practices for ordered lists by:
- Using float values to allow infinite subdivisions between any two positions
- Maintaining large enough gaps between items to minimize rebalancing needs
- Having fallback default values when sort orders are missing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential sort order collisions in the codebase
# Look for places where sort orders are very close together
# Search for sort_order assignments or comparisons
rg -A 3 'sort_order.*=|sort_order.*<|sort_order.*>' --type typescript
# Search for any rebalancing logic that might already exist
rg -A 5 'rebalance|normalize.*sort' --type typescript
Length of output: 197
Script:
#!/bin/bash
# Let's try without type filtering since typescript extension isn't recognized
# Search for sort order related patterns
rg -A 3 'sort_order.*=|sort_order.*<|sort_order.*>'
# Search for any rebalancing logic
rg -A 5 'rebalance|normalize.*sort'
# Also look for the specific increment value (1000) usage
rg -A 2 '[\+\-\*\/]\s*1000'
# Look for the getBlockById implementation
ast-grep --pattern 'function getBlockById'
Length of output: 27201
Script:
#!/bin/bash
# Let's search for the specific increment value (1000) with proper escaping
rg -A 2 '[+\-*/]\s*1000'
# Look for sort order calculations and potential edge cases
rg -A 3 'sort_order.*[+\-*/]'
# Search for getBlockById implementation
rg -A 5 'getBlockById'
Length of output: 187676
web/ce/store/issue/issue-details/activity.store.ts (2)
15-16: LGTM: Import changes align with store refactoring.
The import of CoreRootStore from the core store module aligns with the PR's objective of refactoring timeline stores for better code splitting.
50-50: Verify store functionality access after type change.
The change from RootStore to CoreRootStore looks good, but let's verify that all required store functionalities are still accessible through CoreRootStore.
✅ Verification successful
Let me gather more information about CoreRootStore to ensure it's properly defined.
Let me check one more thing to understand the CoreRootStore definition since we see it's imported from "@/store/root.store".
Based on the shell script results, I can now provide a final response since I have enough information about CoreRootStore and its usage:
Type change from RootStore to CoreRootStore is safe and properly implemented
The change is verified to be correct because:
- CoreRootStore contains the required
issue.issueDetail.commentstructure that's being accessed in the activity store - All store accesses in the file (
this.store.issue.issueDetail.comment) are compatible with CoreRootStore's structure - RootStore extends CoreRootStore, so the type change is a safe narrowing of the interface
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that CoreRootStore provides all necessary functionality used in this class
# Test 1: Check if CoreRootStore contains the required issue store structure
ast-grep --pattern 'export (type|interface|class) CoreRootStore {
$$$
issue: {
$$$
issueDetail: {
$$$
comment: $$$
}
$$$
}
$$$
}'
# Test 2: Look for any other usages of store properties in this file
rg -A 2 'this\.store\.' web/ce/store/issue/issue-details/activity.store.ts
Length of output: 528
Script:
#!/bin/bash
# Search for CoreRootStore definition and its inheritance/implementation
ast-grep --pattern 'export (type|interface|class) CoreRootStore'
# Also check for any type definitions or interfaces that CoreRootStore implements/extends
rg -A 3 'CoreRootStore' web/ce/store
Length of output: 1635
Script:
#!/bin/bash
# Find the actual CoreRootStore definition in the main store directory
fd root.store.ts --exec cat {}
# Also check if there are any interfaces that CoreRootStore implements
ast-grep --pattern 'interface ICoreRootStore'
Length of output: 37291
web/core/store/issue/root.store.ts (4)
6-6: LGTM! Import change aligns with store refactoring.
The import of CoreRootStore is correctly specified and aligns with the PR's objective of refactoring stores.
110-110: Verify CoreRootStore property access.
The autorun block accesses multiple properties from rootStore. Please ensure that all these properties (user, router, state, label, memberRoot, projectRoot, module, cycle) exist in CoreRootStore.
Let's verify the CoreRootStore structure:
✅ Verification successful
Let me verify the autorun block's property access by first checking what properties it uses.
Based on the CoreRootStore definition and the autorun block's property access, I can now provide the final response:
All required properties exist in CoreRootStore
The autorun block accesses the following properties from rootStore which are all present in CoreRootStore:
user: Line 52router: Line 47state: Line 43label: Line 44memberRoot: Line 35projectRoot: Line 34module: Line 38cycle: Line 36
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CoreRootStore definition and properties
ast-grep --pattern 'class CoreRootStore {
$$$
}'
Length of output: 9929
Script:
#!/bin/bash
# Find the autorun block in the issue store to see what properties it accesses from rootStore
ast-grep --pattern 'autorun(() => {
$$$
})'
Length of output: 3266
52-52: Verify the impact of interface type change.
The change from RootStore to CoreRootStore is a breaking change that could affect consumers of IIssueRootStore.
Let's verify the usage:
✅ Verification successful
The previous search didn't yield results for implementations, so let's try a broader search to understand the interface and its usage. We'll also look for references to the old type RootStore to understand the impact of the change.
Based on the search results, I can now provide a conclusive response about the type change from RootStore to CoreRootStore.
Type change is safe and intentional
The change from RootStore to CoreRootStore is actually a safe change as RootStore extends CoreRootStore in both web and admin implementations:
- In web:
web/ce/store/root.store.ts:export class RootStore extends CoreRootStore - In admin:
admin/ce/store/root.store.ts:export class RootStore extends CoreRootStore
The property type change to CoreRootStore makes the interface more precise by using the base class type, while still being compatible with RootStore through inheritance. This follows the Liskov Substitution Principle where a base class can be used in place of any of its derived classes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for implementations and usages of IIssueRootStore
rg "implements IIssueRootStore|: IIssueRootStore" -A 2
Length of output: 9929
Script:
#!/bin/bash
# Search for IIssueRootStore interface definition and its usages
rg "interface IIssueRootStore|IIssueRootStore" -A 2
# Search for RootStore references to understand the type change
rg "RootStore" -A 2
Length of output: 66027
Line range hint 148-224: Verify store initialization compatibility.
The constructor initializes multiple store instances with this as a parameter. Since this now contains a CoreRootStore, verify that all these store classes (IssueStore, StateStore, IssueDetail, etc.) are compatible with the new type.
Let's check the store constructors:
✅ Verification successful
Let me analyze the constructors of the stores that are initialized in the issue root store. I need to check their parameter types.
Let me try a different approach to check the store class definitions.
Let me check the base classes and interfaces these stores implement to fully understand the type requirements.
Based on the analysis of the code, I can now provide a final response:
Store initialization compatibility verified.
The change from RootStore to CoreRootStore in the constructor is compatible with all initialized store instances because:
- All filter stores (
WorkspaceIssuesFilter,ProjectIssuesFilter, etc.) extendIssueFilterHelperStorewhich doesn't require any root store type in its constructor. - Issue stores (
WorkspaceIssues,ProjectIssues, etc.) extendBaseIssuesStoreand receive the root store through their filter store implementations. - The
StateStoreexplicitly acceptsCoreRootStorein its constructor. - The
IssueStoreandIssueDetailimplementations work with theIIssueRootStoreinterface, which defines the required properties that are present inCoreRootStore.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for store class constructors
rg "constructor.*\(.*\)" -A 2 --type ts
Length of output: 35139
Script:
#!/bin/bash
# Search for store class definitions and their constructors
ast-grep --pattern 'class $className {
constructor($params) {
$$$
}
$$$
}'
Length of output: 86
Script:
#!/bin/bash
# Search for IssueStore, StateStore, and other store class definitions
rg "^(export )?class (IssueStore|StateStore|IssueDetail|WorkspaceIssuesFilter|WorkspaceIssues|ProfileIssuesFilter|ProfileIssues|WorkspaceDraftIssuesFilter|WorkspaceDraftIssues|ProjectIssuesFilter|ProjectIssues|CycleIssuesFilter|CycleIssues|ModuleIssuesFilter|ModuleIssues|ProjectViewIssuesFilter|ProjectViewIssues|ArchivedIssuesFilter|ArchivedIssues|DraftIssuesFilter|DraftIssues)" -A 5 --type ts
Length of output: 12381
Script:
#!/bin/bash
# Search for BaseIssuesStore and IssueFilterHelperStore definitions and their constructors
rg "class (BaseIssuesStore|IssueFilterHelperStore)" -B 2 -A 10 --type ts
# Search for relevant interfaces
rg "interface (IIssueRootStore|IBaseIssueFilterStore)" -B 2 -A 10 --type ts
Length of output: 5443
web/ce/store/timeline/base-timeline.store.ts (3)
30-30: LGTM: Clean interface extension.
The addition of isDependencyEnabled to the interface aligns well with the refactoring goals, providing a clear contract for dependency-related functionality.
185-187: LGTM: Proper null handling.
The use of the nullish coalescing operator for sort_order and other fields is appropriate and consistent with TypeScript best practices.
18-18: Verify the impact of nullable sort_order.
The change from number to number | null for sort_order provides more flexibility but ensure that sorting operations handle the null case appropriately.
✅ Verification successful
Based on the search results, I can now generate the final response since I have comprehensive information about how sort_order is used throughout the codebase:
Nullable sort_order is safe and consistent with codebase patterns
The change from number to number | null for sort_order in base-timeline.store.ts is safe because:
-
The codebase already handles null sort orders in multiple places:
- Project models explicitly define
sort_order: number | nullin types - UI components have null handling:
tooltipContent={sort_order === null ? "Join the project to rearrange" : "Drag to rearrange"} - Sorting operations use nullish coalescing:
sort_order ?? undefined,sort_order || 0
- Project models explicitly define
-
The sorting logic across the codebase consistently handles nullable sort orders:
- Default values are provided:
sort_order: number = 65535 - Null checks are implemented:
currentSortOrder = joinedProjects[destinationIndex].sort_order || 0 - Sorting functions use safe accessors:
sortBy(projects, [(p) => p.sort_order])
- Default values are provided:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for sort_order usage in sorting operations
rg -A 3 "sort_order" | rg -B 1 "sort|order"
Length of output: 32446
| export enum ETimeLineTypeType { | ||
| ISSUE = "ISSUE", | ||
| MODULE = "MODULE", | ||
| PROJECT = "PROJECT", |
There was a problem hiding this comment.
💡 Codebase verification
Timeline store integration needs attention
The verification revealed that while the PROJECT enum is being used, its implementation is incomplete:
- The
projectTimeLineStoreis 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
ProjectTimeLineStoreclass extendingTimelineStore - 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
| constructor(rootStore: RootStore) { | ||
| this.issuesTimeLineStore = new IssuesTimeLineStore(rootStore); | ||
| this.modulesTimeLineStore = new ModulesTimeLineStore(rootStore); | ||
| // Dummy store | ||
| this.projectTimeLineStore = new BaseTimeLineStore(rootStore); | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
| 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); | |
| } |
| 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"; |
There was a problem hiding this comment.
💡 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:
-
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
-
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
| isDependencyEnabled = false; | ||
|
|
There was a problem hiding this comment.
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
3 participants
This PR refactors timeline stores to enable effective code splitting
Summary by CodeRabbit
Release Notes
New Features
PROJECTtype.Bug Fixes
sort_ordervalues to prevent errors when they are undefined.Refactor
Chores