Skip to content

[WEB-2001] feat: Fix local cache issues r3 #5714

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

[WEB-2001] feat: Fix local cache issues r3 #5714

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
be8f069
- Handle single quotes in load workspace queries
SatishGandham Sep 26, 2024
215889c
Fix description_html being lost
SatishGandham Sep 26, 2024
00c48c3
Change secondary order to sequence_id
SatishGandham Sep 27, 2024
5cece0f
Fix update persistence layer
SatishGandham Sep 27, 2024
44a2244
Fix issue types filter
SatishGandham Sep 27, 2024
f8305a6
Merge branch 'preview' of https://github.com/makeplane/plane into fix…
SatishGandham Sep 27, 2024
1bc6a0b
add local cache toggle in help section
rahulramesha Sep 27, 2024
aaa59c1
remove toggle from user settings
rahulramesha Sep 27, 2024
594ea31
Reset storage class on disabling local
SatishGandham Sep 27, 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
61 changes: 5 additions & 56 deletions web/app/profile/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,39 +5,20 @@ import { observer } from "mobx-react";
import { Controller, useForm } from "react-hook-form";
import { ChevronDown, CircleUserRound } from "lucide-react";
import { Disclosure, Transition } from "@headlessui/react";
// services
// hooks
// layouts
// components
import type { IUser } from "@plane/types";
import {
Button,
CustomSelect,
CustomSearchSelect,
Input,
TOAST_TYPE,
setPromiseToast,
setToast,
ToggleSwitch,
} from "@plane/ui";
import { Button, CustomSelect, CustomSearchSelect, Input, TOAST_TYPE, setPromiseToast, setToast } from "@plane/ui";
// components
import { DeactivateAccountModal } from "@/components/account";
import { LogoSpinner } from "@/components/common";
import { ImagePickerPopover, UserImageUploadModal, PageHead } from "@/components/core";
// ui
// icons
// components
// constants
import { ProfileSettingContentWrapper } from "@/components/profile";
// constants
import { TIME_ZONES } from "@/constants/timezones";
import { USER_ROLES } from "@/constants/workspace";
// hooks
import { useUser, useUserSettings } from "@/hooks/store";
// import { ProfileSettingsLayout } from "@/layouts/settings-layout";
// layouts
import { ENABLE_LOCAL_DB_CACHE } from "@/plane-web/constants/issues";
import { FileService } from "@/services/file.service";
import { useUser } from "@/hooks/store";
// services
// types
import { FileService } from "@/services/file.service";

const defaultValues: Partial<IUser> = {
avatar: "",
Expand Down Expand Up @@ -69,7 +50,6 @@ const ProfileSettingsPage = observer(() => {
} = useForm<IUser>({ defaultValues });
// store hooks
const { data: currentUser, updateCurrentUser } = useUser();
const { canUseLocalDB, toggleLocalDB } = useUserSettings();

useEffect(() => {
reset({ ...defaultValues, ...currentUser });
Expand Down Expand Up @@ -418,37 +398,6 @@ const ProfileSettingsPage = observer(() => {
</div>
</div>
</form>
{ENABLE_LOCAL_DB_CACHE && (
<Disclosure as="div" className="border-t border-custom-border-100 md:px-8">
{({ open }) => (
<>
<Disclosure.Button as="button" type="button" className="flex w-full items-center justify-between py-4">
<span className="text-lg tracking-tight">Local Cache</span>
<ChevronDown className={`h-5 w-5 transition-all ${open ? "rotate-180" : ""}`} />
</Disclosure.Button>
<Transition
show={open}
enter="transition duration-100 ease-out"
enterFrom="transform opacity-0"
enterTo="transform opacity-100"
leave="transition duration-75 ease-out"
leaveFrom="transform opacity-100"
leaveTo="transform opacity-0"
>
<Disclosure.Panel>
<div className="flex justify-between pb-4">
<span className="text-sm tracking-tight">
Toggled on by default to keep Plane performant. Disable this if you are facing any issues with
Plane. Applicable only to this device.
</span>
<ToggleSwitch value={canUseLocalDB} onChange={() => toggleLocalDB()} />
</div>
</Disclosure.Panel>
</Transition>
</>
)}
</Disclosure>
)}
<Disclosure as="div" className="border-t border-custom-border-100 md:px-8">
{({ open }) => (
<>
Expand Down
25 changes: 23 additions & 2 deletions web/core/components/workspace/sidebar/help-section.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,33 @@

import React, { useState } from "react";
import { observer } from "mobx-react";
import { useParams } from "next/navigation";
import { FileText, HelpCircle, MessagesSquare, MoveLeft, User } from "lucide-react";
// ui
import { CustomMenu, Tooltip } from "@plane/ui";
import { CustomMenu, ToggleSwitch, Tooltip } from "@plane/ui";
// helpers
import { cn } from "@/helpers/common.helper";
// hooks
import { useAppTheme, useCommandPalette, useInstance, useTransient } from "@/hooks/store";
import { useAppTheme, useCommandPalette, useInstance, useTransient, useUserSettings } from "@/hooks/store";
import { usePlatformOS } from "@/hooks/use-platform-os";
// plane web components
import { PlaneVersionNumber, ProductUpdates, ProductUpdatesModal } from "@/plane-web/components/global";
import { WorkspaceEditionBadge } from "@/plane-web/components/workspace";
import { ENABLE_LOCAL_DB_CACHE } from "@/plane-web/constants/issues";

export interface WorkspaceHelpSectionProps {
setSidebarActive?: React.Dispatch<React.SetStateAction<boolean>>;
}

export const SidebarHelpSection: React.FC<WorkspaceHelpSectionProps> = observer(() => {
const { workspaceSlug, projectId } = useParams();
// store hooks
const { sidebarCollapsed, toggleSidebar } = useAppTheme();
const { toggleShortcutModal } = useCommandPalette();
const { isMobile } = usePlatformOS();
const { config } = useInstance();
const { isIntercomToggle, toggleIntercom } = useTransient();
const { canUseLocalDB, toggleLocalDB } = useUserSettings();
// states
const [isNeedHelpOpen, setIsNeedHelpOpen] = useState(false);
const [isChangeLogOpen, setIsChangeLogOpen] = useState(false);
Expand Down Expand Up @@ -105,6 +109,23 @@ export const SidebarHelpSection: React.FC<WorkspaceHelpSectionProps> = observer(
</a>
</CustomMenu.MenuItem>
<div className="my-1 border-t border-custom-border-200" />
{ENABLE_LOCAL_DB_CACHE && (
<CustomMenu.MenuItem>
<div
onClick={(e) => {
e.preventDefault();
e.stopPropagation();
}}
className="flex w-full items-center justify-between text-xs hover:bg-custom-background-80"
>
<span className="racking-tight">Local Cache</span>
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 27, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Typo in className 'racking-tight'

The className racking-tight appears to be a typo. It should likely be tracking-tight, which adjusts letter spacing.

Apply this diff to fix the typo:

-<span className="racking-tight">Local Cache</span>
+<span className="tracking-tight">Local Cache</span>
📝 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
<span className="racking-tight">Local Cache</span>
<span className="tracking-tight">Local Cache</span>

<ToggleSwitch
value={canUseLocalDB}
onChange={() => toggleLocalDB(workspaceSlug?.toString(), projectId?.toString())}
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 27, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle potential 'undefined' values when calling .toString()

When using optional chaining with .toString(), as in workspaceSlug?.toString(), if workspaceSlug is undefined, calling .toString() on it will result in a TypeError because undefined does not have a toString() method. Consider providing default values or handling undefined cases to prevent potential runtime errors.

Apply this diff to safely handle undefined values:

-onChange={() => toggleLocalDB(workspaceSlug?.toString(), projectId?.toString())}
+onChange={() => toggleLocalDB(workspaceSlug ? workspaceSlug.toString() : '', projectId ? projectId.toString() : '')}

Alternatively, ensure that workspaceSlug and projectId are defined before calling .toString().

📝 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
onChange={() => toggleLocalDB(workspaceSlug?.toString(), projectId?.toString())}
onChange={() => toggleLocalDB(workspaceSlug ? workspaceSlug.toString() : '', projectId ? projectId.toString() : '')}

/>
</div>
</CustomMenu.MenuItem>
)}
<CustomMenu.MenuItem>
<button
type="button"
Expand Down
1 change: 1 addition & 0 deletions web/core/local-db/storage.sqlite.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ export class Storage {
const fileSystemDirectoryHandle = await storageManager.getDirectory();
//@ts-expect-error , clear local issue cache
await fileSystemDirectoryHandle.remove({ recursive: true });
this.reset();
} catch (e) {
console.error("Error clearing sqlite sync storage", e);
}
Comment on lines 54 to 60
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 27, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider improving error handling in clearStorage.

The current implementation logs the error but doesn't propagate it. Consider either:

  1. Throwing the error after logging it to allow proper error handling by the caller.
  2. Returning a boolean indicating success or failure.

This would provide better visibility into storage clearing failures.

Here's a suggested implementation:

 clearStorage = async () => {
   try {
     const storageManager = window.navigator.storage;
     const fileSystemDirectoryHandle = await storageManager.getDirectory();
     //@ts-expect-error , clear local issue cache
     await fileSystemDirectoryHandle.remove({ recursive: true });
     this.reset();
+    return true;
   } catch (e) {
     console.error("Error clearing sqlite sync storage", e);
+    return false;
   }
 };
📝 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 fileSystemDirectoryHandle = await storageManager.getDirectory();
//@ts-expect-error , clear local issue cache
await fileSystemDirectoryHandle.remove({ recursive: true });
this.reset();
} catch (e) {
console.error("Error clearing sqlite sync storage", e);
}
const fileSystemDirectoryHandle = await storageManager.getDirectory();
//@ts-expect-error , clear local issue cache
await fileSystemDirectoryHandle.remove({ recursive: true });
this.reset();
return true;
} catch (e) {
console.error("Error clearing sqlite sync storage", e);
return false;
}

Expand Down
14 changes: 9 additions & 5 deletions web/core/local-db/utils/query.utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import { issueSchema } from "./schemas";
import { wrapDateTime } from "./utils";

export const translateQueryParams = (queries: any) => {
const { group_by, sub_group_by, labels, assignees, state, cycle, module, priority, type, ...otherProps } = queries;
const { group_by, sub_group_by, labels, assignees, state, cycle, module, priority, type, issue_type, ...otherProps } =
queries;

const order_by = queries.order_by;
if (state) otherProps.state_id = state;
Expand All @@ -23,6 +24,9 @@ export const translateQueryParams = (queries: any) => {
if (type) {
otherProps.state_group = type === "backlog" ? "backlog" : "unstarted,started";
}
if (issue_type) {
otherProps.type_id = issue_type;
}

if (order_by?.includes("priority")) {
otherProps.order_by = order_by.replace("priority", "priority_proxy");
Expand Down Expand Up @@ -238,11 +242,11 @@ export const singleFilterConstructor = (queries: any) => {

keys.forEach((key) => {
const value = filters[key] ? filters[key].split(",") : "";
if (!value) {
sql += ` AND ${key} IS NULL`;
return;
}
if (!ARRAY_FIELDS.includes(key)) {
if (!value) {
sql += ` AND ${key} IS NULL`;
return;
}
sql += ` AND ${key} in ('${value.join("','")}')
`;
}
Expand Down
12 changes: 9 additions & 3 deletions web/core/store/user/settings.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export interface IUserSettingsStore {
canUseLocalDB: boolean;
// actions
fetchCurrentUserSettings: () => Promise<IUserSettings | undefined>;
toggleLocalDB: () => Promise<void>;
toggleLocalDB: (workspaceSlug: string | undefined, projectId: string | undefined) => Promise<void>;
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 27, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use optional parameter syntax for cleaner code

In TypeScript, it's more idiomatic to use the optional parameter syntax ? for parameters that may be undefined. This enhances readability and conciseness.

Apply this diff to update the method signature:

-toggleLocalDB: (workspaceSlug: string | undefined, projectId: string | undefined) => Promise<void>;
+toggleLocalDB: (workspaceSlug?: string, projectId?: string) => Promise<void>;
📝 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
toggleLocalDB: (workspaceSlug: string | undefined, projectId: string | undefined) => Promise<void>;
toggleLocalDB: (workspaceSlug?: string, projectId?: string) => Promise<void>;

}

export class UserSettingsStore implements IUserSettingsStore {
Expand Down Expand Up @@ -59,7 +59,7 @@ export class UserSettingsStore implements IUserSettingsStore {
this.userService = new UserService();
}

toggleLocalDB = async () => {
toggleLocalDB = async (workspaceSlug: string | undefined, projectId: string | undefined) => {
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 27, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Update method signature to use optional parameters

Ensure that the implementation matches the interface update by using the optional parameter syntax in the method definition as well.

Apply this diff to update the method signature:

-toggleLocalDB = async (workspaceSlug: string | undefined, projectId: string | undefined) => {
+toggleLocalDB = async (workspaceSlug?: string, projectId?: string) => {
📝 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
toggleLocalDB = async (workspaceSlug: string | undefined, projectId: string | undefined) => {
toggleLocalDB = async (workspaceSlug?: string, projectId?: string) => {

const currentLocalDBValue = this.canUseLocalDB;
try {
runInAction(() => {
Expand All @@ -70,8 +70,14 @@ export class UserSettingsStore implements IUserSettingsStore {

if (!transactionResult) {
throw new Error("error while toggling local DB");
} else if (currentLocalDBValue) {
}

if (currentLocalDBValue) {
await persistence.clearStorage();
} else if (workspaceSlug) {
await persistence.initialize(workspaceSlug);
persistence.syncWorkspace();
projectId && persistence.syncIssues(projectId);
Comment on lines +75 to +80
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 27, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Await asynchronous operations to ensure proper execution

If persistence.syncWorkspace() and persistence.syncIssues(projectId) are asynchronous functions, you should await them to ensure they complete before proceeding.

Apply this diff to await the asynchronous calls:

-        persistence.syncWorkspace();
-        projectId && persistence.syncIssues(projectId);
+        await persistence.syncWorkspace();
+        if (projectId) {
+          await persistence.syncIssues(projectId);
+        }
📝 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
if (currentLocalDBValue) {
await persistence.clearStorage();
} else if (workspaceSlug) {
await persistence.initialize(workspaceSlug);
persistence.syncWorkspace();
projectId && persistence.syncIssues(projectId);
if (currentLocalDBValue) {
await persistence.clearStorage();
} else if (workspaceSlug) {
await persistence.initialize(workspaceSlug);
await persistence.syncWorkspace();
if (projectId) {
await persistence.syncIssues(projectId);
}

⚠️ Potential issue

Handle cases when 'workspaceSlug' is undefined during initialization

When enabling the local database (currentLocalDBValue is false), if workspaceSlug is not provided, the initialization process is skipped without any notification or error. This might lead to unexpected behavior.

Consider adding validation to ensure that workspaceSlug is provided when enabling the local database, or handle the case appropriately.

Apply this diff to add a warning when workspaceSlug is undefined:

       } else if (workspaceSlug) {
         await persistence.initialize(workspaceSlug);
-        persistence.syncWorkspace();
-        projectId && persistence.syncIssues(projectId);
+        await persistence.syncWorkspace();
+        if (projectId) {
+          await persistence.syncIssues(projectId);
+        }
+      } else {
+        console.warn("Workspace slug is required to initialize local database");
       }
📝 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
if (currentLocalDBValue) {
await persistence.clearStorage();
} else if (workspaceSlug) {
await persistence.initialize(workspaceSlug);
persistence.syncWorkspace();
projectId && persistence.syncIssues(projectId);
if (currentLocalDBValue) {
await persistence.clearStorage();
} else if (workspaceSlug) {
await persistence.initialize(workspaceSlug);
await persistence.syncWorkspace();
if (projectId) {
await persistence.syncIssues(projectId);
}
} else {
console.warn("Workspace slug is required to initialize local database");
}

}
} catch (e) {
console.warn("error while toggling local DB");
Expand Down