improvement: refactored issue grouping logic to access MobX store directly#6134
improvement: refactored issue grouping logic to access MobX store directly#6134
Conversation
prateekshourya29
requested review from
SatishGandham,
rahulramesha and
sriramveeraghanta
WalkthroughThe pull request introduces modifications across several files, primarily focusing on the Changes
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
web/core/components/issues/issue-layouts/kanban/swimlanes.tsx (1)
263-275: LGTM! Improved store access patternThe refactoring simplifies the store access by:
- Consolidating multiple store hooks into a single
useIssueStoreTypehook- Using derived values with named parameters for better readability
- Maintaining type safety with explicit type casting
This aligns well with the PR's objective of simplifying store access patterns.
Consider extracting these derived values into a custom hook for better reusability:
const useGroupByColumns = ( groupBy: TIssueGroupByOptions | undefined, subGroupBy: TIssueGroupByOptions | undefined ) => { const storeType = useIssueStoreType(); const isWorkspaceLvl = isWorkspaceLevel(storeType); return { groupByList: getGroupByColumns({ groupBy: groupBy as GroupByColumnTypes, includeNone: true, isWorkspaceLevel: isWorkspaceLvl, }), subGroupByList: getGroupByColumns({ groupBy: subGroupBy as GroupByColumnTypes, includeNone: true, isWorkspaceLevel: isWorkspaceLvl, }) }; };web/core/store/module.store.ts (1)
289-292: Simplify error handling in catch blocksThe error handling in catch blocks has been simplified by removing the error parameter. While this reduces verbosity, consider adding error tracking for production monitoring.
Consider adding error tracking:
- } catch { + } catch (error) { + if (process.env.NODE_ENV === 'production') { + // Add your error tracking service here + errorTrackingService.captureError(error); + } this.loader = false; return undefined; }Also applies to: 315-318
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
packages/types/src/issues.d.ts(1 hunks)web/core/components/issues/issue-layouts/kanban/default.tsx(2 hunks)web/core/components/issues/issue-layouts/kanban/swimlanes.tsx(1 hunks)web/core/components/issues/issue-layouts/list/default.tsx(1 hunks)web/core/components/issues/issue-layouts/utils.tsx(8 hunks)web/core/store/cycle.store.ts(6 hunks)web/core/store/module.store.ts(5 hunks)
🔇 Additional comments (11)
web/core/components/issues/issue-layouts/utils.tsx (2)
69-107: Refactored getGroupByColumns function enhances clarity
The getGroupByColumns function now accepts a single object parameter, improving readability and maintainability by grouping related arguments.
Line range hint 109-186: Simplified helper functions by accessing store directly
The helper functions now access the store context directly, reducing redundancy and streamlining data retrieval.
web/core/store/cycle.store.ts (3)
53-53: Addition of getProjectCycleDetails method improves cycle retrieval
Introducing the getProjectCycleDetails method enhances the ability to retrieve detailed cycle information for a given project.
328-336: Implemented getProjectCycleDetails function correctly
The function effectively retrieves and sorts cycles for the specified project, ensuring accurate data access.
340-348: Implemented getProjectCycleIds function efficiently
Using getProjectCycleDetails to obtain cycle IDs promotes code reuse and consistency across the codebase.
packages/types/src/issues.d.ts (1)
219-219: Made icon property optional in IGroupByColumn
Updating the icon property to be optional increases flexibility when defining group columns without mandating an icon.
web/core/components/issues/issue-layouts/list/default.tsx (1)
81-85: Simplified getGroupByColumns function call
Refactoring the call to getGroupByColumns to use an object parameter enhances readability and aligns with the updated function signature.
web/core/components/issues/issue-layouts/kanban/default.tsx (1)
90-99: Updated getGroupByColumns function call for consistency
Adjusting the getGroupByColumns call to accept an object parameter simplifies the code and ensures consistency with the refactored function.
web/core/store/module.store.ts (3)
34-34: LGTM! Added new method to interface
The new getProjectModuleDetails method in the interface properly defines the return type and parameter.
214-222: LGTM! Well-implemented computed method
The new getProjectModuleDetails method:
- Properly checks fetch status
- Filters modules by project ID and archived status
- Maintains sort order consistency
224-233: LGTM! Improved code reuse
The refactored getProjectModuleIds method now reuses getProjectModuleDetails, reducing code duplication and improving maintainability.
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| fetchActiveCycleProgressPro = action(async (workspaceSlug: string, projectId: string, cycleId: string) => {}); |
There was a problem hiding this comment.
Incomplete implementation of fetchActiveCycleProgressPro
The method fetchActiveCycleProgressPro is currently empty, indicating incomplete functionality. Please implement this method or remove it if it's no longer needed.
Would you like assistance in implementing this method or should it be removed?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
web/core/store/module.store.ts (1)
295-297: Consider logging errors in thecatchblocksIn the
fetchModulesandfetchModulesSlimmethods, thecatchblocks do not capture or log the errors, which may hinder debugging and error tracing. Consider logging the errors to provide more context in case of failures.Proposed change for
fetchModules:} catch (error) { this.loader = false; + console.error("Failed to fetch modules:", error); return undefined; }Similarly, for
fetchModulesSlim:} catch (error) { this.loader = false; + console.error("Failed to fetch modules slim:", error); return undefined; }Also applies to: 321-323
web/core/components/issues/issue-layouts/utils.tsx (1)
132-159: Potential null check forcurrentProjectDetailsingetCycleColumnsThe function assumes
currentProjectDetailsandcurrentProjectDetails.idare defined. Consider adding null checks or handling undefined cases to prevent potential runtime errors.const { currentProjectDetails } = store.projectRoot.project; if (!currentProjectDetails || !currentProjectDetails.id) return;This check is already present; ensure similar checks are implemented wherever necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
web/core/components/issues/issue-layouts/kanban/swimlanes.tsx(1 hunks)web/core/components/issues/issue-layouts/utils.tsx(8 hunks)web/core/store/cycle.store.ts(6 hunks)web/core/store/module.store.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/core/components/issues/issue-layouts/kanban/swimlanes.tsx
🔇 Additional comments (15)
web/core/store/module.store.ts (3)
35-36: Addition of getProjectModuleDetails method approved
The addition of the getProjectModuleDetails method enhances module retrieval functionality by providing direct access to module details based on projectId.
218-223: Efficient retrieval of project module details
The getProjectModuleDetails function efficiently retrieves and sorts module details for a given projectId. The use of computedFn optimizes performance by memoizing the computed values.
229-233: Refactoring getProjectModuleIds to utilize getProjectModuleDetails
The refactoring of getProjectModuleIds to use getProjectModuleDetails promotes code reuse and simplifies logic. This enhances maintainability and reduces potential errors.
web/core/components/issues/issue-layouts/utils.tsx (6)
39-39: Direct import of store for state management
Importing store directly from @/lib/store-context centralizes state management and aligns with the PR objective of accessing the MobX store directly, simplifying the implementation.
63-66: Improved type definition for getGroupByColumns parameters
Defining TGetGroupByColumns as an object enhances readability and future maintainability by allowing for easier addition of parameters.
73-107: Refactored getGroupByColumns function enhances clarity
The refactoring of getGroupByColumns to accept a single object parameter and the use of a mapping object (groupByColumnMap) improves the clarity and scalability of the function.
232-238: Default parameter in getLabelsColumns and usage of correct label set
The function getLabelsColumns correctly uses the default parameter isWorkspaceLevel: boolean = false. It appropriately selects workspace or project labels based on this parameter.
253-256: Direct access to store in getAssigneeColumns and getCreatedByColumns
Accessing store.memberRoot directly simplifies the functions and aligns with the PR objective. Ensure that projectMemberIds and getUserDetails are available and handle any potential undefined values.
[approve]
Run the following script to verify that all projectMemberIds have corresponding user details:
Also applies to: 269-273
✅ Verification successful
Based on the gathered information, I can now provide the final response:
Direct access to store.memberRoot is safe and properly handled
The code correctly handles potential undefined values:
memberRootis properly typed withIMemberRootStoreinterface which includesmemberMapandgetUserDetails- The function has a guard clause
if (!projectMemberIds) return getUserDetailsis widely used across the codebase and is a standard way to access user information- The pattern aligns with the codebase's conventions as seen in multiple components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all project member IDs have corresponding user details.
# Expected result: All member IDs should have corresponding entries in `store.memberRoot.getUserDetails`.
for memberId in "${store.memberRoot.project.projectMemberIds[@]}"; do
if [[ -z "${store.memberRoot.getUserDetails($memberId)}" ]]; then
echo "Missing user details for member ID: $memberId"
fi
done
Length of output: 136
Script:
#!/bin/bash
# Let's analyze the code structure and usage of store.memberRoot
ast-grep --pattern 'store.memberRoot'
# Also check for any error handling or type definitions related to memberRoot
rg "memberRoot" -A 3
Length of output: 7606
Script:
#!/bin/bash
# Let's examine the type definitions and implementation of memberRoot store
rg "interface IMemberRootStore" -A 10
# Also check the implementation of getUserDetails
ast-grep --pattern 'getUserDetails'
# And check how projectMemberIds is handled
ast-grep --pattern 'projectMemberIds'
Length of output: 14045
Line range hint 110-129: Ensure all projects are loaded before mapping
In the getProjectColumns function, ensure that all projects are loaded into projectMap to prevent undefined entries when mapping project IDs to columns.
Run the following script to verify that all projects are loaded:
🧰 Tools
🪛 Biome (1.9.4)
[error] 61-61: 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)
web/core/store/cycle.store.ts (6)
17-17: Updated imports reflect new helper functions
The import statement now includes orderCycles and shouldFilterCycle from cycle.helper, indicating refactoring and code reuse.
54-54: Addition of getProjectCycleDetails method
The getProjectCycleDetails method provides a streamlined way to retrieve cycle details for a given projectId, enhancing the functionality of the CycleStore.
333-338: Efficient retrieval of project cycle details
The getProjectCycleDetails function efficiently retrieves and sorts cycle details for a given projectId, improving code organization and reusability.
345-350: Refactored getProjectCycleIds to use getProjectCycleDetails
By utilizing getProjectCycleDetails, the getProjectCycleIds function reduces code duplication and enhances maintainability.
415-417: Conditional addition to activeCycleIdMap
The inclusion of a condition to set the activeCycleIdMap ensures only cycles with status "current" are marked active, which improves logic accuracy.
497-498: Incomplete implementation of fetchActiveCycleProgressPro
The method fetchActiveCycleProgressPro is currently empty, indicating incomplete functionality. Please implement this method or remove it if it's no longer needed.
…ectly (makeplane#6134) * improvement: refactored issue grouping logic to access MobX store directly * chore: minor updates
3 participants
Simplified the implementation by accessing the MobX store directly within the grouping logic, eliminating the need to pass it as a parameter. This reduces redundancy and improves code readability.
Other improvements
Summary by CodeRabbit
New Features
iconproperty in theIGroupByColumninterface is now optional, enhancing flexibility.getProjectModuleDetailsmethod for improved module retrieval.Improvements
Kanban,List, and utility components for better clarity and reduced dependencies.Bug Fixes