[WEB-2001] chore: Code refactor for noload changes. #5683
Conversation
… services for modules and cycles
WalkthroughThis pull request introduces several enhancements across various components and stores in the codebase. Key changes include the addition of a new enumeration for server-side identifiers, modifications to how issue details are fetched and managed, and improvements in loading state handling. These adjustments aim to enhance the management and rendering of issue-related data throughout the application. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Files skipped from review due to trivial changes (1)
Files skipped from review as they are similar to previous changes (1)
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 (
|
SatishGandham
changed the title
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (9)
web/core/hooks/use-local-storage.tsx (1)
Line range hint
3-13: LGTM with a minor suggestionThe
getValueFromLocalStoragefunction looks good overall. It handles potential errors and edge cases well.Consider using the nullish coalescing operator (
??) instead of the logical OR (||) for the window check:- if (typeof window === undefined || typeof window === "undefined") return defaultValue; + if (typeof window === undefined ?? typeof window === "undefined") return defaultValue;This change would make the check more precise, as it only considers
undefinedas a falsy value.Tools
Biome
[error] 4-4: Invalid
typeofcomparison value: this expression is not a string literalnot a string literal
Unsafe fix: Compare the result oftypeofwith a valid type name(lint/suspicious/useValidTypeof)
web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/issues/(detail)/[issueId]/page.tsx (2)
44-44: Approve logic change and suggest simplificationThe update to
issueLoaderlogic correctly incorporates the newisFetchingIssueDetailsstate, which is an improvement. However, the boolean expression can be simplified.Consider simplifying the boolean expression:
- const issueLoader = !issue || isFetchingIssueDetails ? true : false; + const issueLoader = !issue || isFetchingIssueDetails;This change removes the unnecessary ternary operator, making the code more concise and easier to read.
Tools
Biome
[error] 44-44: 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)
Line range hint
1-102: Overall assessment: Improved issue detail managementThe changes in this file successfully implement the intended improvements to issue detail fetching and management. The introduction of
isFetchingIssueDetailsand the updates to how loading states are handled create a more consistent and maintainable approach. These modifications align well with the PR objectives of refining issue-related functionality.Consider documenting the new loading state management approach in the project's development guidelines to ensure consistent implementation across the codebase.
web/core/components/issues/issue-layouts/kanban/block.tsx (1)
227-227: LGTM: Proper usage of new prop. Consider adding documentation.The
shouldRenderByDefaultprop is correctly passed to theRenderIfVisiblecomponent asdefaultValue. This usage aligns well with the prop's intended purpose of controlling default rendering behavior.Consider adding a brief comment above the
RenderIfVisiblecomponent to explain the purpose and impact of theshouldRenderByDefaultprop. This would enhance code readability and maintainability.web/core/store/issue/cycle/filter.store.ts (1)
122-127: Approve changes with suggestions for improvementThe modifications to
getFilterParamsenhance its robustness by handling cases wheregetAppliedFiltersmight return a falsy value. This aligns well with the PR objective of modifying how issues are fetched for cycles.Suggestions for improvement:
- Consider adding a comment explaining why
getAppliedFiltersmight return a falsy value, as this would improve code maintainability.Consider adding a brief comment explaining the potential falsy return value from
getAppliedFilters:let filterParams = this.getAppliedFilters(cycleId); +// Ensure filterParams is an object in case getAppliedFilters returns a falsy value if (!filterParams) { filterParams = {}; } filterParams["cycle"] = cycleId;web/core/store/issue/module/filter.store.ts (1)
122-127: Approve changes with a minor suggestion for improvementThe changes improve the robustness of the
getFilterParamsmethod by handling the case wherefilterParamsmight be undefined. This prevents potential errors and ensures that themoduleIdis always included in the filter parameters.To improve clarity and make the code more concise, consider using the nullish coalescing operator:
let filterParams = this.getAppliedFilters(moduleId) ?? {}; filterParams["module"] = moduleId;This achieves the same result but in a more succinct manner.
web/core/components/issues/peek-overview/root.tsx (2)
Line range hint
55-67: LGTM: Simplified loading state management in fetch functionThe removal of local loading state management aligns with the centralized approach using
isFetchingIssueDetails. This change simplifies the code and reduces the risk of inconsistent loading states.However, consider enhancing the error handling:
Consider adding more specific error handling:
} catch (error) { setError(true); - console.error("Error fetching the parent issue"); + console.error("Error fetching the parent issue:", error); + // Optionally, you could use a toast or other notification method here }This change would provide more detailed error information for debugging purposes.
347-347: LGTM: Consistent loading state in IssueViewThe use of
isFetchingIssueDetailsfor theisLoadingprop ensures that theIssueViewcomponent accurately reflects the current loading state of issue details. This change improves consistency and reliability of the UI.For improved consistency across the component, consider renaming the prop to match the state variable:
-isLoading={isFetchingIssueDetails} +isFetchingIssueDetails={isFetchingIssueDetails}This change would make the prop name more descriptive and align it with the state variable name, potentially improving code readability and maintainability.
web/core/store/issue/cycle/issue.store.ts (1)
Line range hint
1-424: Summary of changes and recommendationsThe modifications in this file align with the PR objective of updating the
getIssuescall for cycles to use the project'sgetIssuesAPI. However, there are a few points to consider:
- Ensure that the
paramsobject includes the necessary cycle information, as thecycleIdparameter has been removed from some method calls.- Address the inconsistency in the method call at line 236, where
cycleIdis still included as a parameter.- Review the entire file to ensure that all occurrences of
getCycleIssueshave been updated consistently.- Update any related tests or documentation to reflect these changes.
Consider creating a helper method within the
CycleIssuesclass to encapsulate the logic for callingthis.issueService.getIssueswith the correct parameters. This would help maintain consistency across different method calls and make future updates easier.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- packages/constants/issue.ts (1 hunks)
- web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/archives/issues/(detail)/[archivedIssueId]/page.tsx (2 hunks)
- web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/issues/(detail)/[issueId]/page.tsx (2 hunks)
- web/core/components/issues/issue-layouts/kanban/block.tsx (3 hunks)
- web/core/components/issues/issue-layouts/kanban/blocks-list.tsx (2 hunks)
- web/core/components/issues/peek-overview/root.tsx (2 hunks)
- web/core/hooks/use-local-storage.tsx (2 hunks)
- web/core/store/issue/cycle/filter.store.ts (1 hunks)
- web/core/store/issue/cycle/issue.store.ts (2 hunks)
- web/core/store/issue/issue-details/issue.store.ts (6 hunks)
- web/core/store/issue/module/filter.store.ts (1 hunks)
- web/core/store/issue/module/issue.store.ts (2 hunks)
Additional context used
Biome
web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/archives/issues/(detail)/[archivedIssueId]/page.tsx
[error] 43-43: 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/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/issues/(detail)/[issueId]/page.tsx
[error] 44-44: 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/hooks/use-local-storage.tsx
[error] 15-15: Invalid
typeofcomparison value: this expression is not a string literalnot a string literal
Unsafe fix: Compare the result oftypeofwith a valid type name(lint/suspicious/useValidTypeof)
Additional comments not posted (21)
packages/constants/issue.ts (1)
16-27: LGTM: New enumEIssueGroupBYServerToPropertyaddedThe new enum
EIssueGroupBYServerToPropertyprovides a mapping from server-side property names to their corresponding identifiers. This addition enhances the type safety and maintainability of the codebase when working with server-side properties.web/core/hooks/use-local-storage.tsx (2)
Line range hint
24-56: LGTM: Hook implementation remains consistentThe
useLocalStoragehook remains unchanged and correctly utilizes the exportedgetValueFromLocalStoragefunction. Its implementation is consistent with the changes made to the utility functions.Tools
Biome
[error] 15-15: Invalid
typeofcomparison value: this expression is not a string literalnot a string literal
Unsafe fix: Compare the result oftypeofwith a valid type name(lint/suspicious/useValidTypeof)
Line range hint
1-56: Summary: Improved reusability and consistency in local storage managementThe changes in this file enhance the reusability of local storage utility functions by exporting them. This aligns well with the PR objectives of refining existing functionality and improving efficiency.
Key improvements:
getValueFromLocalStorageis now exported, allowing its use in other parts of the application.- A new
setValueIntoLocalStoragefunction has been added, providing a consistent way to store values in local storage.- The
useLocalStoragehook remains unchanged, maintaining its existing functionality while benefiting from the exported utility functions.These changes contribute to a more modular and maintainable codebase, potentially simplifying issue management across the application.
Tools
Biome
[error] 15-15: Invalid
typeofcomparison value: this expression is not a string literalnot a string literal
Unsafe fix: Compare the result oftypeofwith a valid type name(lint/suspicious/useValidTypeof)
web/core/components/issues/issue-layouts/kanban/blocks-list.tsx (3)
40-40: LGTM: Addition of index parameterThe addition of the
indexparameter to themapfunction is a good change. It allows for index-based conditional rendering of issues, which aligns with the PR objective of adjusting the default rendering of issues in the Kanban view.
53-53: Verify: Number of issues rendered by defaultThe new
shouldRenderByDefaultprop is set totruefor indices 0 through 10, which means 11 issues will be rendered by default. This slightly differs from the PR objective of displaying 10 issues per column.Could you please clarify if this is intentional? If not, consider adjusting the condition to
index < 10to render exactly 10 issues by default.
Line range hint
40-53: Summary: Implementation of default issue rendering in Kanban viewThe changes effectively implement the PR objective of adjusting the default rendering of issues in the Kanban view. By adding the
indexparameter and theshouldRenderByDefaultprop, the component now has a mechanism to control which issues are rendered by default.This implementation allows for lazy loading of issues, which can significantly improve performance for large Kanban boards. Users will see the first set of issues immediately, with the ability to load more as needed.
Overall, these changes enhance the efficiency and user experience of the Kanban view, aligning well with the PR objectives.
web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/archives/issues/(detail)/[archivedIssueId]/page.tsx (1)
22-22: Improved issue loading state trackingThe addition of
isFetchingIssueDetailsto the destructuredissueobject provides a more granular way to track the loading state of issue details. This aligns well with the PR objective of updating the logic for fetching issue details and should lead to more accurate loading indicators in the UI.web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/issues/(detail)/[issueId]/page.tsx (2)
30-30: LGTM: Improved issue detail loading state trackingThe addition of
isFetchingIssueDetailsenhances the component's ability to track the loading state of issue details more accurately. This change aligns well with the PR's objective of refining issue management functionality.
Line range hint
35-41: LGTM: Simplified loading state managementThe removal of
isLoadingfrom theuseSWRhook's destructured properties streamlines the component's state management. This change, in conjunction with the addition ofisFetchingIssueDetails, centralizes the loading state logic, making the code more maintainable and consistent.web/core/components/issues/issue-layouts/kanban/block.tsx (2)
42-42: LGTM: New prop added to control default rendering.The addition of the optional
shouldRenderByDefaultprop to theIssueBlockPropsinterface is a good enhancement. It provides more control over the rendering behavior of the issue block while maintaining backward compatibility.
Line range hint
1-240: Summary: Enhancing Kanban issue block rendering controlThe changes in this file introduce a new
shouldRenderByDefaultprop to theKanbanIssueBlockcomponent, providing more granular control over the default rendering behavior of issue blocks. This enhancement aligns well with the PR objectives of improving issue handling and presentation in the Kanban view.Key points:
- The new prop is optional, maintaining backward compatibility.
- It's correctly implemented in both the interface and component usage.
- The change potentially allows for performance optimizations by controlling which issues are rendered by default.
Overall, these changes are well-implemented and contribute positively to the flexibility of the Kanban issue layout.
web/core/store/issue/module/issue.store.ts (3)
Line range hint
1-280: Overall assessment: Changes improve consistency and align with PR objectives.The modifications in this file successfully implement the PR objective of unifying the
getIssuesAPI call. The changes are consistent and should improve code maintainability. However, it's crucial to ensure that themoduleIdis correctly included in theparamsobject for both initial fetching and pagination.To ensure the changes don't introduce any regressions, please run the existing test suite for this module and verify that all tests pass.
193-193: LGTM! Verify params object includes moduleId for pagination.This change is consistent with the previous modification, using
this.issueService.getIssuesfor fetching the next page of issues. This maintains consistency across the codebase.Please ensure that the
paramsobject includes themoduleIdfor pagination. You can verify this by running the following script:#!/bin/bash # Description: Verify that moduleId is included in the params object for getIssues call in pagination # Test: Search for the getFilterParams method call for pagination in the file rg --type typescript -A 10 'getFilterParams.*moduleId.*this\.getNextCursor' web/core/store/issue/module/issue.store.ts
145-147: LGTM! Verify params object includes moduleId.The change to use
this.issueService.getIssuesinstead ofthis.moduleService.getModuleIssuesaligns with the PR objective of unifying the issue fetching API. This should improve consistency across the codebase.Please ensure that the
paramsobject includes themoduleId. You can verify this by running the following script:Verification successful
Verified:
paramsincludemoduleId.The
paramsobject is correctly constructed withmoduleIdincluded, ensuring consistent filtering in thegetIssuesAPI call.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that moduleId is included in the params object for getIssues call # Test: Search for the getFilterParams method call in the file rg --type typescript -A 10 'getFilterParams.*moduleId' web/core/store/issue/module/issue.store.tsLength of output: 155
Script:
#!/bin/bash # Description: Verify that moduleId is included in the params object for getIssues call # Test: Search for the getFilterParams method call in the file with the correct file type rg --type ts -A 10 'getFilterParams.*moduleId' web/core/store/issue/module/issue.store.tsLength of output: 660
web/core/components/issues/peek-overview/root.tsx (1)
38-38: LGTM: Improved loading state managementThe addition of
isFetchingIssueDetailsto the destructuredissueobject aligns with the centralized state management approach. This change eliminates the need for a local loading state and provides a more consistent way to track the loading status of issue details across the component.web/core/store/issue/issue-details/issue.store.ts (6)
1-1: Appropriate Import of MobX UtilitiesThe import of
makeObservableandobservablefrom"mobx"is necessary for making class properties observable.
35-35: Addition of isFetchingIssueDetails to IIssueStore InterfaceAdding
isFetchingIssueDetails: boolean;to theIIssueStoreinterface correctly defines the new observable property.
41-41: Initialization of isFetchingIssueDetails in IssueStore
isFetchingIssueDetailsis initialized tofalse, which is appropriate for tracking the fetching state.
50-52: Proper Declaration of Observable PropertyUsing
observable.refforisFetchingIssueDetailscorrectly makes the primitive boolean observable in MobX.
73-73: Updating Type Annotation for issue VariableDeclaring
issueaslet issue: TIssue | undefined;ensures that the variable correctly represents a potential undefined value if the fetch fails.
85-85: Refactoring with addIssueToStore MethodUtilizing
this.addIssueToStore(issue);enhances code readability by encapsulating the logic for adding an issue to the store.
| export enum EIssueGroupBYServerToProperty { | ||
| "state_id" = "state_id", | ||
| "priority" = "priority", | ||
| "labels__id" = "label_ids", | ||
| "state__group" = "state__group", | ||
| "assignees__id" = "assignee_ids", | ||
| "cycle_id" = "cycle_id", | ||
| "issue_module__module_id" = "module_ids", | ||
| "target_date" = "target_date", | ||
| "project_id" = "project_id", | ||
| "created_by" = "created_by", | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Inconsistent Naming Conventions Identified
The enums EIssueGroupBYServerToProperty and EIssueGroupByToServerOptions exhibit inconsistent naming conventions, which can lead to confusion and maintenance challenges.
- Unused Enum:
EIssueGroupBYServerToPropertyis defined inpackages/constants/issue.tsbut does not appear to be used elsewhere in the codebase. - Naming Discrepancies:
labels__idmaps tolabel_idsinstead oflabels__idassignees__idmaps toassignee_idsinstead ofassignees__idissue_module__module_idmaps tomodule_idsinstead ofissue_module__module_id
Recommendation:
- Align the naming conventions between
EIssueGroupBYServerToPropertyandEIssueGroupByToServerOptions. - Remove the unused
EIssueGroupBYServerToPropertyenum if it is deprecated to maintain clarity.
Analysis chain
Verify naming consistency with existing enums
There are some differences in naming conventions between EIssueGroupBYServerToProperty and EIssueGroupByToServerOptions:
labels__idmaps tolabel_idsinstead oflabels__idassignees__idmaps toassignee_idsinstead ofassignees__idissue_module__module_idmaps tomodule_idsinstead ofissue_module__module_id
Please verify if these differences are intentional. If not, consider aligning the naming conventions to maintain consistency across the codebase.
To help verify the usage of these properties, you can run the following script:
This script will help identify how the new enum is being used in comparison to the existing ones, and whether the naming differences are consistent throughout the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of the new enum and compare with existing ones
# Search for usages of EIssueGroupBYServerToProperty
echo "Usages of EIssueGroupBYServerToProperty:"
rg --type typescript "EIssueGroupBYServerToProperty" -A 5
echo "\nUsages of EIssueGroupByToServerOptions:"
rg --type typescript "EIssueGroupByToServerOptions" -A 5
echo "\nUsages of labels__id, assignees__id, and issue_module__module_id:"
rg --type typescript "labels__id|assignees__id|issue_module__module_id" -A 5
Length of output: 627
Script:
#!/bin/bash
# Description: Search for usages of the enums and specific property names in TypeScript files
# Search for usages of EIssueGroupBYServerToProperty in .ts and .tsx files
echo "Usages of EIssueGroupBYServerToProperty:"
rg "EIssueGroupBYServerToProperty" -A 5 -g "*.ts" -g "*.tsx"
echo "\nUsages of EIssueGroupByToServerOptions:"
rg "EIssueGroupByToServerOptions" -A 5 -g "*.ts" -g "*.tsx"
echo "\nUsages of labels__id, assignees__id, and issue_module__module_id:"
rg "labels__id|assignees__id|issue_module__module_id" -A 5 -g "*.ts" -g "*.tsx"
Length of output: 24627
| export const setValueIntoLocalStorage = (key: string, value: any) => { | ||
| if (typeof window === undefined || typeof window === "undefined") return false; | ||
| try { | ||
| window.localStorage.setItem(key, JSON.stringify(value)); | ||
| return true; | ||
| } catch (error) { | ||
| return false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Approve with a fix for the typeof comparison
The setValueIntoLocalStorage function is a good addition. It properly handles potential errors and edge cases.
However, there's an issue with the typeof comparison. Let's fix it:
- if (typeof window === undefined || typeof window === "undefined") return false;
+ if (typeof window === "undefined") return false;This change addresses the static analysis warning and makes the check more concise and correct.
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 setValueIntoLocalStorage = (key: string, value: any) => { | |
| if (typeof window === undefined || typeof window === "undefined") return false; | |
| try { | |
| window.localStorage.setItem(key, JSON.stringify(value)); | |
| return true; | |
| } catch (error) { | |
| return false; | |
| } | |
| }; | |
| export const setValueIntoLocalStorage = (key: string, value: any) => { | |
| if (typeof window === "undefined") return false; | |
| try { | |
| window.localStorage.setItem(key, JSON.stringify(value)); | |
| return true; | |
| } catch (error) { | |
| return false; | |
| } | |
| }; |
Tools
Biome
[error] 15-15: Invalid
typeofcomparison value: this expression is not a string literalnot a string literal
Unsafe fix: Compare the result oftypeofwith a valid type name(lint/suspicious/useValidTypeof)
| @@ -40,7 +40,7 @@ const ArchivedIssueDetailsPage = observer(() => { | |||
|
|
|||
| if (!issue) return <></>; | |||
|
|
|||
| const issueLoader = !issue || isLoading ? true : false; | |||
| const issueLoader = !issue || isFetchingIssueDetails ? true : false; | |||
There was a problem hiding this comment.
Approve logic change, but simplify the expression
The updated issueLoader assignment improves the accuracy of the loading state by considering both the presence of the issue and the fetching state. This aligns with the PR objective of updating the logic for fetching issue details.
However, the expression can be simplified:
-const issueLoader = !issue || isFetchingIssueDetails ? true : false;
+const issueLoader = !issue || isFetchingIssueDetails;This simplification removes the unnecessary ternary operator, making the code more concise and easier to read while maintaining the same functionality.
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 issueLoader = !issue || isFetchingIssueDetails ? true : false; | |
| const issueLoader = !issue || isFetchingIssueDetails; |
Tools
Biome
[error] 43-43: 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)
| @@ -233,7 +233,7 @@ export class CycleIssues extends BaseIssuesStore implements ICycleIssues { | |||
| subGroupId | |||
| ); | |||
| // call the fetch issues API with the params for next page in issues | |||
| const response = await this.cycleService.getCycleIssues(workspaceSlug, projectId, cycleId, params); | |||
| const response = await this.issueService.getIssues(workspaceSlug, projectId, cycleId, params); | |||
There was a problem hiding this comment.
Inconsistency in method call parameters
While the change aligns with the PR objective of using the getIssues API, there's an inconsistency with the previous change at line 188.
To maintain consistency, consider removing the cycleId parameter from this method call as well. Apply this diff:
- const response = await this.issueService.getIssues(workspaceSlug, projectId, cycleId, params);
+ const response = await this.issueService.getIssues(workspaceSlug, projectId, params);Also, ensure that the cycleId is included in the params object if it's required for filtering issues.
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 response = await this.issueService.getIssues(workspaceSlug, projectId, cycleId, params); | |
| const response = await this.issueService.getIssues(workspaceSlug, projectId, params); |
| const response = await this.issueService.getIssues(workspaceSlug, projectId, params, { | ||
| signal: this.controller.signal, | ||
| }); |
There was a problem hiding this comment.
Tip
Codebase Verification
Cycle ID Still Passed in getFilterParams
The cycleId is still being passed to the getFilterParams method. This may lead to inconsistencies with the updated getIssues call.
const params = this.issueFilterStore?.getFilterParams(options, cycleId, undefined, undefined, undefined);
Analysis chain
LGTM! Verify cycle information in params
The change aligns with the PR objective of modifying the getIssues call for cycles to use the same getIssues API of the project. This should help in streamlining the process of fetching issues.
Please ensure that the params object includes the cycle information, as the cycleId parameter has been removed from the method call. Run the following script to verify:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the params object includes cycle information
# Test: Search for the getFilterParams method call in the file
rg --type typescript 'getFilterParams.*cycleId' web/core/store/issue/cycle/issue.store.ts
Length of output: 126
Script:
#!/bin/bash
# Description: Verify that the params object includes cycle information
# Test: Search for the getFilterParams method call with cycleId in TypeScript files
rg 'getFilterParams.*cycleId' -g '*.ts' web/core/store/issue/cycle/issue.store.ts
Length of output: 195
| addIssueToStore = (issue: TIssue) => { | ||
| const issuePayload: TIssue = { | ||
| id: issue?.id, | ||
| sequence_id: issue?.sequence_id, | ||
| name: issue?.name, | ||
| description_html: issue?.description_html, | ||
| sort_order: issue?.sort_order, | ||
| state_id: issue?.state_id, | ||
| priority: issue?.priority, | ||
| label_ids: issue?.label_ids, | ||
| assignee_ids: issue?.assignee_ids, | ||
| estimate_point: issue?.estimate_point, | ||
| sub_issues_count: issue?.sub_issues_count, | ||
| attachment_count: issue?.attachment_count, | ||
| link_count: issue?.link_count, | ||
| project_id: issue?.project_id, | ||
| parent_id: issue?.parent_id, | ||
| cycle_id: issue?.cycle_id, | ||
| module_ids: issue?.module_ids, | ||
| type_id: issue?.type_id, | ||
| created_at: issue?.created_at, | ||
| updated_at: issue?.updated_at, | ||
| start_date: issue?.start_date, | ||
| target_date: issue?.target_date, | ||
| completed_at: issue?.completed_at, | ||
| archived_at: issue?.archived_at, | ||
| created_by: issue?.created_by, | ||
| updated_by: issue?.updated_by, | ||
| is_draft: issue?.is_draft, | ||
| is_subscribed: issue?.is_subscribed, | ||
| }; |
There was a problem hiding this comment.
Simplify issuePayload Creation Using Object Spread Operator
Manually copying each property from issue to issuePayload is verbose and error-prone. Simplify the assignment by using the object spread operator to clone the issue object.
Apply this diff to refactor:
addIssueToStore = (issue: TIssue) => {
- const issuePayload: TIssue = {
- id: issue?.id,
- sequence_id: issue?.sequence_id,
- name: issue?.name,
- description_html: issue?.description_html,
- sort_order: issue?.sort_order,
- state_id: issue?.state_id,
- priority: issue?.priority,
- label_ids: issue?.label_ids,
- assignee_ids: issue?.assignee_ids,
- estimate_point: issue?.estimate_point,
- sub_issues_count: issue?.sub_issues_count,
- attachment_count: issue?.attachment_count,
- link_count: issue?.link_count,
- project_id: issue?.project_id,
- parent_id: issue?.parent_id,
- cycle_id: issue?.cycle_id,
- module_ids: issue?.module_ids,
- type_id: issue?.type_id,
- created_at: issue?.created_at,
- updated_at: issue?.updated_at,
- start_date: issue?.start_date,
- target_date: issue?.target_date,
- completed_at: issue?.completed_at,
- archived_at: issue?.archived_at,
- created_by: issue?.created_by,
- updated_by: issue?.updated_by,
- is_draft: issue?.is_draft,
- is_subscribed: issue?.is_subscribed,
- };
+ const issuePayload: TIssue = { ...issue };This refactor improves maintainability by reducing code duplication and potential for errors.
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.
| addIssueToStore = (issue: TIssue) => { | |
| const issuePayload: TIssue = { | |
| id: issue?.id, | |
| sequence_id: issue?.sequence_id, | |
| name: issue?.name, | |
| description_html: issue?.description_html, | |
| sort_order: issue?.sort_order, | |
| state_id: issue?.state_id, | |
| priority: issue?.priority, | |
| label_ids: issue?.label_ids, | |
| assignee_ids: issue?.assignee_ids, | |
| estimate_point: issue?.estimate_point, | |
| sub_issues_count: issue?.sub_issues_count, | |
| attachment_count: issue?.attachment_count, | |
| link_count: issue?.link_count, | |
| project_id: issue?.project_id, | |
| parent_id: issue?.parent_id, | |
| cycle_id: issue?.cycle_id, | |
| module_ids: issue?.module_ids, | |
| type_id: issue?.type_id, | |
| created_at: issue?.created_at, | |
| updated_at: issue?.updated_at, | |
| start_date: issue?.start_date, | |
| target_date: issue?.target_date, | |
| completed_at: issue?.completed_at, | |
| archived_at: issue?.archived_at, | |
| created_by: issue?.created_by, | |
| updated_by: issue?.updated_by, | |
| is_draft: issue?.is_draft, | |
| is_subscribed: issue?.is_subscribed, | |
| }; | |
| addIssueToStore = (issue: TIssue) => { | |
| const issuePayload: TIssue = { ...issue }; |
| let issue: TIssue; | ||
| let issue: TIssue | undefined; | ||
|
|
||
| this.isFetchingIssueDetails = true; |
There was a problem hiding this comment.
Ensure isFetchingIssueDetails is Reset on Error
isFetchingIssueDetails is set to true before fetching the issue, but if an error occurs before addIssueToStore is called, it may remain true, leading to an incorrect loading state. Wrap the fetch logic in a try...finally block to ensure isFetchingIssueDetails is reset regardless of success or failure.
Apply this diff to fix the issue:
fetchIssue = async (workspaceSlug: string, projectId: string, issueId: string, issueType = "DEFAULT") => {
const query = {
expand: "issue_reactions,issue_attachment,issue_link,parent",
};
let issue: TIssue | undefined;
this.isFetchingIssueDetails = true;
+ try {
if (issueType === "ARCHIVED")
issue = await this.issueArchiveService.retrieveArchivedIssue(workspaceSlug, projectId, issueId, query);
else if (issueType === "DRAFT")
issue = await this.issueDraftService.getDraftIssueById(workspaceSlug, projectId, issueId, query);
else
issue = await this.issueService.retrieve(workspaceSlug, projectId, issueId, query);
if (!issue) throw new Error("Issue not found");
this.addIssueToStore(issue);
+ } finally {
+ this.isFetchingIssueDetails = false;
+ }
return issue;
};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.
| this.isFetchingIssueDetails = true; | |
| fetchIssue = async (workspaceSlug: string, projectId: string, issueId: string, issueType = "DEFAULT") => { | |
| const query = { | |
| expand: "issue_reactions,issue_attachment,issue_link,parent", | |
| }; | |
| let issue: TIssue | undefined; | |
| this.isFetchingIssueDetails = true; | |
| try { | |
| if (issueType === "ARCHIVED") | |
| issue = await this.issueArchiveService.retrieveArchivedIssue(workspaceSlug, projectId, issueId, query); | |
| else if (issueType === "DRAFT") | |
| issue = await this.issueDraftService.getDraftIssueById(workspaceSlug, projectId, issueId, query); | |
| else | |
| issue = await this.issueService.retrieve(workspaceSlug, projectId, issueId, query); | |
| if (!issue) throw new Error("Issue not found"); | |
| this.addIssueToStore(issue); | |
| } finally { | |
| this.isFetchingIssueDetails = false; | |
| } | |
| return issue; | |
| }; |
| @@ -185,7 +185,7 @@ export class CycleIssues extends BaseIssuesStore implements ICycleIssues { | |||
| // get params from pagination options | |||
| const params = this.issueFilterStore?.getFilterParams(options, cycleId, undefined, undefined, undefined); | |||
| // call the fetch issues API with the params | |||
| const response = await this.cycleService.getCycleIssues(workspaceSlug, projectId, cycleId, params, { | |||
| const response = await this.issueService.getIssues(workspaceSlug, projectId, params, { | |||
There was a problem hiding this comment.
You should add the cycle id to the params.
There was a problem hiding this comment.
That is being handled in the const params = this.issueFilterStore?.getFilterParams at line 186
| @@ -11,6 +11,16 @@ const getValueFromLocalStorage = (key: string, defaultValue: any) => { | |||
| } | |||
| }; | |||
|
|
|||
| export const setValueIntoLocalStorage = (key: string, value: any) => { | |||
| if (typeof window === undefined || typeof window === "undefined") return false; | |||
There was a problem hiding this comment.
It was just a check that was copied over from the previous method getValueFromLocalStorage. The second check only should be good. Left the first one there as i was not sure, couldn't find references on the first check. Should i change it to only the second condition?
| @@ -142,7 +142,7 @@ export class ModuleIssues extends BaseIssuesStore implements IModuleIssues { | |||
| // get params from pagination options | |||
| const params = this.issueFilterStore?.getFilterParams(options, moduleId, undefined, undefined, undefined); | |||
| // call the fetch issues API with the params | |||
| const response = await this.moduleService.getModuleIssues(workspaceSlug, projectId, moduleId, params, { | |||
| const response = await this.issueService.getIssues(workspaceSlug, projectId, params, { | |||
There was a problem hiding this comment.
You should pass module id in params.
There was a problem hiding this comment.
Same as Cycle params handled within getFilterParams
This PR separates out additional no load changes, these changes include:
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style