Skip to content

[WEB-2545] fix: peek overview loading state #5698

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
pushya22 merged 1 commit into from
Sep 26, 2024
Merged

[WEB-2545] fix: peek overview loading state #5698

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

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 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,7 +19,7 @@ const ArchivedIssueDetailsPage = observer(() => {
// hooks
const {
fetchIssue,
issue: { getIssueById, isFetchingIssueDetails },
issue: { getIssueById, getIsFetchingIssueDetails },
} = useIssueDetail();

const { getProjectById } = useProject();
Expand All @@ -40,7 +40,7 @@ const ArchivedIssueDetailsPage = observer(() => {

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

const issueLoader = !issue || isFetchingIssueDetails ? true : false;
const issueLoader = !issue || getIsFetchingIssueDetails(issue?.id) ? true : false;

return (
<>
Expand Down
4 changes: 2 additions & 2 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,7 +27,7 @@ const IssueDetailsPage = observer(() => {
// store hooks
const {
fetchIssue,
issue: { getIssueById, isFetchingIssueDetails },
issue: { getIssueById, getIsFetchingIssueDetails },
} = useIssueDetail();
const { getProjectById } = useProject();
const { toggleIssueDetailSidebar, issueDetailSidebarCollapsed } = useAppTheme();
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 || isFetchingIssueDetails ? true : false;
const issueLoader = !issue || getIsFetchingIssueDetails(issue?.id) ? 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/issues/peek-overview/root.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export const IssuePeekOverview: FC<IIssuePeekOverview> = observer((props) => {
const {
peekIssue,
setPeekIssue,
issue: { fetchIssue, isFetchingIssueDetails },
issue: { fetchIssue, getIsFetchingIssueDetails },
fetchActivities,
} = useIssueDetail();

Expand Down Expand Up @@ -344,7 +344,7 @@ export const IssuePeekOverview: FC<IIssuePeekOverview> = observer((props) => {
workspaceSlug={peekIssue.workspaceSlug}
projectId={peekIssue.projectId}
issueId={peekIssue.issueId}
isLoading={isFetchingIssueDetails}
isLoading={getIsFetchingIssueDetails(peekIssue.issueId)}
isError={error}
is_archived={is_archived}
disabled={!isEditable}
Expand Down
4 changes: 3 additions & 1 deletion web/core/components/issues/peek-overview/view.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export const IssueView: FC<IIssueView> = observer((props) => {
isArchiveIssueModalOpen,
toggleDeleteIssueModal,
toggleArchiveIssueModal,
issue: { getIssueById, isLocalDBIssueDescription },
issue: { getIssueById, getIsLocalDBIssueDescription },
} = useIssueDetail();
const issue = getIssueById(issueId);
// remove peek id
Expand All @@ -69,6 +69,8 @@ export const IssueView: FC<IIssueView> = observer((props) => {
if (embedIssue) embedRemoveCurrentNotification && embedRemoveCurrentNotification();
};

const isLocalDBIssueDescription = getIsLocalDBIssueDescription(issueId);

usePeekOverviewOutsideClickDetector(
issuePeekOverviewRef,
() => {
Expand Down
31 changes: 22 additions & 9 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
Expand Up @@ -34,15 +34,15 @@ export interface IIssueStoreActions {
}

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

export class IssueStore implements IIssueStore {
isFetchingIssueDetails: boolean = false;
isLocalDBIssueDescription: boolean = false;
fetchingIssueDetails: string | undefined = undefined;
localDBIssueDescription: string | undefined = undefined;
Comment on lines +44 to +45
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 25, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Potential issue with concurrent fetching of issue details

The properties fetchingIssueDetails and localDBIssueDescription are currently defined to store a single issueId (string | undefined). If multiple issues are fetched concurrently, this implementation may not accurately reflect the loading state for each issue, leading to race conditions or incorrect UI states.

Consider modifying these properties to hold a collection of issueIds, such as a Set<string>, to properly track the loading state of multiple issues concurrently.

// root store
rootIssueDetailStore: IIssueDetail;
// services
Expand All @@ -52,7 +52,8 @@ export class IssueStore implements IIssueStore {

constructor(rootStore: IIssueDetail) {
makeObservable(this, {
isFetchingIssueDetails: observable.ref,
fetchingIssueDetails: observable.ref,
localDBIssueDescription: observable.ref,
});
// root store
this.rootIssueDetailStore = rootStore;
Expand All @@ -62,6 +63,18 @@ export class IssueStore implements IIssueStore {
this.issueDraftService = new IssueDraftService();
}

getIsFetchingIssueDetails = computedFn((issueId: string | undefined) => {
if (!issueId) return false;

return this.fetchingIssueDetails === issueId;
});

getIsLocalDBIssueDescription = computedFn((issueId: string | undefined) => {
if (!issueId) return false;

return this.localDBIssueDescription === issueId;
});

// helper methods
getIssueById = computedFn((issueId: string) => {
if (!issueId) return undefined;
Expand All @@ -79,11 +92,11 @@ export class IssueStore implements IIssueStore {
// fetch issue from local db
issue = await persistence.getIssue(issueId);

this.isFetchingIssueDetails = true;
this.fetchingIssueDetails = issueId;

if (issue) {
this.addIssueToStore(issue);
this.isLocalDBIssueDescription = true;
this.localDBIssueDescription = issueId;
Comment on lines +95 to +99
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 25, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure accurate tracking of fetching states for multiple issues

When initiating the fetch operation, you set this.fetchingIssueDetails = issueId;. With multiple concurrent fetches, you should add the issueId to the collection rather than assigning it directly. Similarly, when an issue is fetched from the local DB, add the issueId to localDBIssueDescription to accurately reflect the loading states.

}

if (issueType === "ARCHIVED")
Expand All @@ -95,7 +108,7 @@ export class IssueStore implements IIssueStore {
if (!issue) throw new Error("Issue not found");

const issuePayload = this.addIssueToStore(issue);
this.isLocalDBIssueDescription = false;
this.localDBIssueDescription = undefined;
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 25, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Properly clear the local DB issue description state

After processing the issue, you set this.localDBIssueDescription = undefined;. If multiple issues are being processed, this might prematurely reset the state. Ensure you remove only the relevant issueId from the collection to maintain accurate state.


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

Expand Down Expand Up @@ -173,7 +186,7 @@ export class IssueStore implements IIssueStore {
};

this.rootIssueDetailStore.rootIssueStore.issues.addIssue([issuePayload]);
this.isFetchingIssueDetails = false;
this.fetchingIssueDetails = undefined;
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 25, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Properly clear the fetching issue details state

At the end of addIssueToStore, you set this.fetchingIssueDetails = undefined;. If multiple fetch operations are in progress, this could inadvertently reset the fetching state for other issues. Consider removing only the completed issueId from the collection.


return issuePayload;
};
Expand Down