Skip to content

[WEB-2001] chore: Code refactor for noload changes. #5683

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
SatishGandham merged 9 commits into from
Sep 24, 2024
Merged

[WEB-2001] chore: Code refactor for noload changes. #5683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
45936e9
use common getIssues from issue service instead of multiple different…
rahulramesha Jul 29, 2024
33dc931
add group by to server constants
rahulramesha Sep 23, 2024
e0f0a09
change issue detail's overview's is loading logic to the loader from …
rahulramesha Sep 23, 2024
d6f6333
add extra method in local storage
rahulramesha Sep 23, 2024
bbecefb
Kanban render 10 issues by default per column
rahulramesha Sep 23, 2024
3c0b5bf
Merge branch 'preview' of github.com:makeplane/plane into preview
rahulramesha Sep 23, 2024
d279013
fix height in group virtualization
rahulramesha Sep 23, 2024
7d3b799
remove debounced code for Kanban fetching more issues per column
rahulramesha Sep 24, 2024
abf341b
fix lint errors
rahulramesha Sep 24, 2024
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
13 changes: 13 additions & 0 deletions packages/constants/issue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,19 @@ export enum EIssueGroupByToServerOptions {
"created_by" = "created_by",
}

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",
}
Comment on lines +16 to +27
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 23, 2024

Choose a reason for hiding this comment

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

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: EIssueGroupBYServerToProperty is defined in packages/constants/issue.ts but does not appear to be used elsewhere in the codebase.
  • Naming Discrepancies:
    • labels__id maps to label_ids instead of labels__id
    • assignees__id maps to assignee_ids instead of assignees__id
    • issue_module__module_id maps to module_ids instead of issue_module__module_id

Recommendation:

  • Align the naming conventions between EIssueGroupBYServerToProperty and EIssueGroupByToServerOptions.
  • Remove the unused EIssueGroupBYServerToProperty enum 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:

  1. labels__id maps to label_ids instead of labels__id
  2. assignees__id maps to assignee_ids instead of assignees__id
  3. issue_module__module_id maps to module_ids instead of issue_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 enum EServerGroupByToFilterOptions {
"state_id" = "state",
"priority" = "priority",
Expand Down
6 changes: 3 additions & 3 deletions ...ojects)/projects/(detail)/[projectId]/archives/issues/(detail)/[archivedIssueId]/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@ const ArchivedIssueDetailsPage = observer(() => {
// hooks
const {
fetchIssue,
issue: { getIssueById },
issue: { getIssueById, isFetchingIssueDetails },
} = useIssueDetail();

const { getProjectById } = useProject();

const { isLoading } = useSWR(
useSWR(
workspaceSlug && projectId && archivedIssueId
? `ARCHIVED_ISSUE_DETAIL_${workspaceSlug}_${projectId}_${archivedIssueId}`
: null,
Expand All @@ -40,7 +40,7 @@ const ArchivedIssueDetailsPage = observer(() => {

if (!issue) return <></>;

const issueLoader = !issue || isLoading ? true : false;
const issueLoader = !issue || isFetchingIssueDetails ? true : false;
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 23, 2024

Choose a reason for hiding this comment

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

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.

Suggested change
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)


return (
<>
Expand Down
6 changes: 3 additions & 3 deletions ...orkspaceSlug]/(projects)/projects/(detail)/[projectId]/issues/(detail)/[issueId]/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@ const IssueDetailsPage = observer(() => {
// store hooks
const {
fetchIssue,
issue: { getIssueById },
issue: { getIssueById, isFetchingIssueDetails },
} = useIssueDetail();
const { getProjectById } = useProject();
const { toggleIssueDetailSidebar, issueDetailSidebarCollapsed } = useAppTheme();
// fetching issue details
const { isLoading, error } = useSWR(
const { error } = useSWR(
workspaceSlug && projectId && issueId ? `ISSUE_DETAIL_${workspaceSlug}_${projectId}_${issueId}` : null,
workspaceSlug && projectId && issueId
? () => fetchIssue(workspaceSlug.toString(), projectId.toString(), issueId.toString())
Expand All @@ -41,7 +41,7 @@ const IssueDetailsPage = observer(() => {
// derived values
const issue = getIssueById(issueId?.toString() || "") || undefined;
const project = (issue?.project_id && getProjectById(issue?.project_id)) || undefined;
const issueLoader = !issue || isLoading ? true : false;
const issueLoader = !issue || isFetchingIssueDetails ? true : false;
const pageTitle = project && issue ? `${project?.identifier}-${issue?.sequence_id} ${issue?.name}` : undefined;

useEffect(() => {
Expand Down
4 changes: 2 additions & 2 deletions web/core/components/common/logo.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@
import { FC } from "react";
// emoji-picker-react
import { Emoji } from "emoji-picker-react";
// import { icons } from "lucide-react";
import useFontFaceObserver from "use-font-face-observer";
import { TLogoProps } from "@plane/types";
// helpers
import { LUCIDE_ICONS_LIST } from "@plane/ui";
import { emojiCodeToUnicode } from "@/helpers/emoji.helper";
// import { icons } from "lucide-react";
import useFontFaceObserver from "use-font-face-observer";

type Props = {
logo: TLogoProps;
Expand Down
9 changes: 1 addition & 8 deletions web/core/components/issues/issue-layouts/kanban/base-kanban-root.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import { FC, useCallback, useEffect, useRef, useState } from "react";
import { combine } from "@atlaskit/pragmatic-drag-and-drop/combine";
import { dropTargetForElements } from "@atlaskit/pragmatic-drag-and-drop/element/adapter";
import { autoScrollForElements } from "@atlaskit/pragmatic-drag-and-drop-auto-scroll/element";
import debounce from "lodash/debounce";
import { observer } from "mobx-react";
import { useParams, usePathname } from "next/navigation";
import { DeleteIssueModal } from "@/components/issues";
Expand Down Expand Up @@ -93,12 +92,6 @@ export const BaseKanBanRoot: React.FC<IBaseKanBanLayout> = observer((props: IBas
[fetchNextIssues]
);

const debouncedFetchMoreIssues = debounce(
(groupId?: string, subgroupId?: string) => fetchMoreIssues(groupId, subgroupId),
300,
{ leading: true, trailing: false }
);

const groupedIssueIds = issues?.groupedIssueIds;

const userDisplayFilters = displayFilters || null;
Expand Down Expand Up @@ -275,7 +268,7 @@ export const BaseKanBanRoot: React.FC<IBaseKanBanLayout> = observer((props: IBas
addIssuesToView={addIssuesToView}
scrollableContainerRef={scrollableContainerRef}
handleOnDrop={handleOnDrop}
loadMoreIssues={debouncedFetchMoreIssues}
loadMoreIssues={fetchMoreIssues}
/>
</div>
</div>
Expand Down
3 changes: 3 additions & 0 deletions web/core/components/issues/issue-layouts/kanban/block.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ interface IssueBlockProps {
quickActions: TRenderQuickActions;
canEditProperties: (projectId: string | undefined) => boolean;
scrollableContainerRef?: MutableRefObject<HTMLDivElement | null>;
shouldRenderByDefault?: boolean;
}

interface IssueDetailsBlockProps {
Expand Down Expand Up @@ -114,6 +115,7 @@ export const KanbanIssueBlock: React.FC<IssueBlockProps> = observer((props) => {
quickActions,
canEditProperties,
scrollableContainerRef,
shouldRenderByDefault,
} = props;

const cardRef = useRef<HTMLAnchorElement | null>(null);
Expand Down Expand Up @@ -222,6 +224,7 @@ export const KanbanIssueBlock: React.FC<IssueBlockProps> = observer((props) => {
defaultHeight="100px"
horizontalOffset={100}
verticalOffset={200}
defaultValue={shouldRenderByDefault}
>
<KanbanIssueDetailsBlock
cardRef={cardRef}
Expand Down
3 changes: 2 additions & 1 deletion web/core/components/issues/issue-layouts/kanban/blocks-list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export const KanbanIssueBlocksList: React.FC<IssueBlocksListProps> = observer((p
<>
{issueIds && issueIds.length > 0 ? (
<>
{issueIds.map((issueId) => {
{issueIds.map((issueId, index) => {
if (!issueId) return null;

let draggableId = issueId;
Expand All @@ -50,6 +50,7 @@ export const KanbanIssueBlocksList: React.FC<IssueBlocksListProps> = observer((p
issueId={issueId}
groupId={groupId}
subGroupId={sub_group_id}
shouldRenderByDefault={index <= 10}
issuesMap={issuesMap}
displayProperties={displayProperties}
updateIssue={updateIssue}
Expand Down
2 changes: 1 addition & 1 deletion web/core/components/issues/issue-layouts/kanban/default.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ export const KanBan: React.FC<IKanBan> = observer((props) => {
verticalOffset={100}
horizontalOffset={100}
root={scrollableContainerRef}
classNames="relative h-full"
classNames="h-full min-h-[120px]"
defaultHeight={`${groupHeight}px`}
placeholderChildren={
<KanbanColumnLoader
Expand Down
8 changes: 2 additions & 6 deletions web/core/components/issues/peek-overview/root.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,13 @@ export const IssuePeekOverview: FC<IIssuePeekOverview> = observer((props) => {
const {
peekIssue,
setPeekIssue,
issue: { fetchIssue },
issue: { fetchIssue, isFetchingIssueDetails },
fetchActivities,
} = useIssueDetail();

const { issues } = useIssuesStore();
const { captureIssueEvent } = useEventTracker();
// state
const [loader, setLoader] = useState(true);
const [error, setError] = useState(false);

const removeRoutePeekId = () => {
Expand All @@ -54,18 +53,15 @@ export const IssuePeekOverview: FC<IIssuePeekOverview> = observer((props) => {
() => ({
fetch: async (workspaceSlug: string, projectId: string, issueId: string, loader = true) => {
try {
setLoader(loader);
setError(false);
await fetchIssue(
workspaceSlug,
projectId,
issueId,
is_archived ? "ARCHIVED" : is_draft ? "DRAFT" : "DEFAULT"
);
setLoader(false);
setError(false);
} catch (error) {
setLoader(false);
setError(true);
console.error("Error fetching the parent issue");
}
Expand Down Expand Up @@ -348,7 +344,7 @@ export const IssuePeekOverview: FC<IIssuePeekOverview> = observer((props) => {
workspaceSlug={peekIssue.workspaceSlug}
projectId={peekIssue.projectId}
issueId={peekIssue.issueId}
isLoading={loader}
isLoading={isFetchingIssueDetails}
isError={error}
is_archived={is_archived}
disabled={!isEditable}
Expand Down
12 changes: 11 additions & 1 deletion web/core/hooks/use-local-storage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { useState, useEffect, useCallback } from "react";

const getValueFromLocalStorage = (key: string, defaultValue: any) => {
export const getValueFromLocalStorage = (key: string, defaultValue: any) => {
if (typeof window === undefined || typeof window === "undefined") return defaultValue;
try {
const item = window.localStorage.getItem(key);
Expand All @@ -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;
Copy link
Contributor

@SatishGandham SatishGandham Sep 23, 2024

Choose a reason for hiding this comment

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

Why two checks?

Copy link
Contributor Author

@rahulramesha rahulramesha Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

try {
window.localStorage.setItem(key, JSON.stringify(value));
return true;
} catch (error) {
return false;
}
};
Comment on lines +14 to +22
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 23, 2024

Choose a reason for hiding this comment

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

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.

Suggested change
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 typeof comparison value: this expression is not a string literal

not a string literal
Unsafe fix: Compare the result of typeof with a valid type name

(lint/suspicious/useValidTypeof)


const useLocalStorage = <T,>(key: string, initialValue: T) => {
const [storedValue, setStoredValue] = useState<T | null>(() => getValueFromLocalStorage(key, initialValue));

Expand Down
7 changes: 6 additions & 1 deletion web/core/store/issue/cycle/filter.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,12 @@ export class CycleIssuesFilter extends IssueFilterHelperStore implements ICycleI
groupId: string | undefined,
subGroupId: string | undefined
) => {
const filterParams = this.getAppliedFilters(cycleId);
let filterParams = this.getAppliedFilters(cycleId);

if (!filterParams) {
filterParams = {};
}
filterParams["cycle"] = cycleId;

const paginationParams = this.getPaginationParams(filterParams, options, cursor, groupId, subGroupId);
return paginationParams;
Expand Down
4 changes: 2 additions & 2 deletions web/core/store/issue/cycle/issue.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Copy link
Contributor

@SatishGandham SatishGandham Sep 23, 2024

Choose a reason for hiding this comment

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

You should add the cycle id to the params.

Copy link
Contributor Author

@rahulramesha rahulramesha Sep 23, 2024

Choose a reason for hiding this comment

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

That is being handled in the const params = this.issueFilterStore?.getFilterParams at line 186

signal: this.controller.signal,
});
Comment on lines +188 to 190
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 23, 2024

Choose a reason for hiding this comment

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

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


Expand Down Expand Up @@ -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);
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 23, 2024

Choose a reason for hiding this comment

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

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.

Suggested change
const response = await this.issueService.getIssues(workspaceSlug, projectId, cycleId, params);
const response = await this.issueService.getIssues(workspaceSlug, projectId, params);


// after the next page of issues are fetched, call the base method to process the response
this.onfetchNexIssues(response, groupId, subGroupId);
Expand Down
83 changes: 48 additions & 35 deletions web/core/store/issue/issue-details/issue.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { makeObservable } from "mobx";
import { makeObservable, observable } from "mobx";
import { computedFn } from "mobx-utils";
// types
import { TIssue } from "@plane/types";
Expand Down Expand Up @@ -32,11 +32,13 @@ export interface IIssueStoreActions {
}

export interface IIssueStore extends IIssueStoreActions {
isFetchingIssueDetails: boolean;
// helper methods
getIssueById: (issueId: string) => TIssue | undefined;
}

export class IssueStore implements IIssueStore {
isFetchingIssueDetails: boolean = false;
// root store
rootIssueDetailStore: IIssueDetail;
// services
Expand All @@ -45,7 +47,9 @@ export class IssueStore implements IIssueStore {
issueDraftService;

constructor(rootStore: IIssueDetail) {
makeObservable(this, {});
makeObservable(this, {
isFetchingIssueDetails: observable.ref,
});
// root store
this.rootIssueDetailStore = rootStore;
// services
Expand All @@ -66,7 +70,9 @@ export class IssueStore implements IIssueStore {
expand: "issue_reactions,issue_attachment,issue_link,parent",
};

let issue: TIssue;
let issue: TIssue | undefined;

this.isFetchingIssueDetails = true;
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 23, 2024

Choose a reason for hiding this comment

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

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.

Suggested change
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;
};


if (issueType === "ARCHIVED")
issue = await this.issueArchiveService.retrieveArchivedIssue(workspaceSlug, projectId, issueId, query);
Expand All @@ -76,38 +82,7 @@ export class IssueStore implements IIssueStore {

if (!issue) throw new Error("Issue not found");

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,
};

this.rootIssueDetailStore.rootIssueStore.issues.addIssue([issuePayload]);
this.addIssueToStore(issue);

// store handlers from issue detail
// parent
Expand Down Expand Up @@ -150,6 +125,44 @@ export class IssueStore implements IIssueStore {
return issue;
};

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,
};
Comment on lines +128 to +158
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 23, 2024

Choose a reason for hiding this comment

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

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.

Suggested change
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 };


this.rootIssueDetailStore.rootIssueStore.issues.addIssue([issuePayload]);
this.isFetchingIssueDetails = false;

return issuePayload;
};

updateIssue = async (workspaceSlug: string, projectId: string, issueId: string, data: Partial<TIssue>) => {
await this.rootIssueDetailStore.rootIssueStore.projectIssues.updateIssue(workspaceSlug, projectId, issueId, data);
await this.rootIssueDetailStore.activity.fetchActivities(workspaceSlug, projectId, issueId);
Expand Down
Loading