Skip to content

[WEB-2001] feat: Fix local cache issues r4 #5726

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 7 commits into from
Oct 1, 2024
Merged

[WEB-2001] feat: Fix local cache issues r4 #5726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 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
53e9d64
Add instrumentation
SatishGandham Sep 27, 2024
8ae412e
- Fallback to server incase of any error
SatishGandham Sep 30, 2024
e16421e
Merge branch 'preview' of https://github.com/makeplane/plane into loc…
SatishGandham Sep 30, 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
46 changes: 40 additions & 6 deletions web/core/local-db/storage.sqlite.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as Sentry from "@sentry/nextjs";
import set from "lodash/set";
// plane
import { EIssueGroupBYServerToProperty } from "@plane/constants";
Expand All @@ -15,7 +16,7 @@ import { loadWorkSpaceData } from "./utils/load-workspace";
import { issueFilterCountQueryConstructor, issueFilterQueryConstructor } from "./utils/query-constructor";
import { runQuery } from "./utils/query-executor";
import { createTables } from "./utils/tables";
import { logError, getGroupedIssueResults, getSubGroupedIssueResults, logInfo, log } from "./utils/utils";
import { getGroupedIssueResults, getSubGroupedIssueResults, log, logError, logInfo } from "./utils/utils";

declare module "@sqlite.org/sqlite-wasm" {
export function sqlite3Worker1Promiser(...args: any): any;
Expand Down Expand Up @@ -67,7 +68,7 @@ export class Storage {
this.reset();
}
try {
await this._initialize(workspaceSlug);
await Sentry.startSpan({ name: "INIT_DB" }, async () => await this._initialize(workspaceSlug));
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 30, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Inconsistent usage of Sentry.startSpan

The Sentry.startSpan method is used inconsistently across the codebase:

  • Line 71:

    await Sentry.startSpan({ name: "INIT_DB" }, async () => await this._initialize(workspaceSlug));

    Used with await and an async callback.

  • Lines 152-154:

    await Sentry.startSpan({ name: "LOAD_WS", attributes: { slug: this.workspaceSlug } }, async () => {
  await loadWorkSpaceData(this.workspaceSlug);
});

    Similar usage with await and an async callback.

  • Line 179:

    const sync = Sentry.startSpan({ name: `SYNC_ISSUES` }, () => this._syncIssues(projectId));

    Here, Sentry.startSpan is called without await, and the result is assigned to const sync.

Recommendations:

  • Ensure consistent usage of Sentry.startSpan. Decide whether to await the function call and whether to use an async callback consistently.
  • Verify that Sentry.startSpan is the correct method to use. According to the Sentry SDK documentation, you might need to use Sentry.startTransaction or create a span from an existing transaction.
  • Check if Sentry.startSpan accepts a callback function. If not, you may need to manually start and finish the span.

Also applies to: 152-154, 179-179

return true;
} catch (err) {
logError(err);
Expand Down Expand Up @@ -148,7 +149,9 @@ export class Storage {

syncWorkspace = async () => {
if (document.hidden || !rootStore.user.localDBEnabled) return; // return if the window gets hidden
loadWorkSpaceData(this.workspaceSlug);
await Sentry.startSpan({ name: "LOAD_WS", attributes: { slug: this.workspaceSlug } }, async () => {
await loadWorkSpaceData(this.workspaceSlug);
});
};

syncProject = async (projectId: string) => {
Expand All @@ -173,7 +176,7 @@ export class Storage {
if (document.hidden || !rootStore.user.localDBEnabled) return false; // return if the window gets hidden

try {
const sync = this._syncIssues(projectId);
const sync = Sentry.startSpan({ name: `SYNC_ISSUES` }, () => this._syncIssues(projectId));
this.setSync(projectId, sync);
await sync;
} catch (e) {
Expand All @@ -183,6 +186,8 @@ export class Storage {
};

_syncIssues = async (projectId: string) => {
const activeSpan = Sentry.getActiveSpan();

Comment on lines +189 to +190
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 30, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Potential issues with active span management

In several places, you retrieve the active span and set attributes:

  • Lines 189-190:

    const activeSpan = Sentry.getActiveSpan();

  • Lines 251-254:

    activeSpan?.setAttributes({
  projectId: projectId,
  count: response.total_count,
});

  • Lines 366-376:

    const activeSpan = Sentry.getActiveSpan();
activeSpan?.setAttributes({
  projectId,
  count: total_count,
  groupBy: group_by,
  subGroupBy: sub_group_by,
  queries: queries,
  local: true,
  groupCount,
  subGroupCount,
});

Concerns:

  • If there is no active span, activeSpan will be undefined, and the attributes won't be set. This could lead to missing data in your tracing.
  • Setting attributes after asynchronous operations might not associate the data with the correct span.

Recommendations:

  • Ensure that an active span exists before attempting to set attributes.
  • Consider passing the span explicitly or using span contexts to maintain consistency.
  • Verify that setting attributes at these points correctly captures the intended data.

Also applies to: 251-254, 366-376

log("### Sync started");
let status = this.getStatus(projectId);
if (status === "loading" || status === "syncing") {
Expand Down Expand Up @@ -242,6 +247,11 @@ export class Storage {
this.setOption(projectId, "ready");
this.setStatus(projectId, "ready");
this.setSync(projectId, undefined);

activeSpan?.setAttributes({
projectId: projectId,
count: response.total_count,
});
};

getIssueCount = async (projectId: string) => {
Expand Down Expand Up @@ -279,7 +289,9 @@ export class Storage {
currentProjectStatus === "error" ||
!rootStore.user.localDBEnabled
) {
logInfo(`Project ${projectId} is loading, falling back to server`);
if (rootStore.user.localDBEnabled) {
logInfo(`Project ${projectId} is loading, falling back to server`);
}
const issueService = new IssueService();
return await issueService.getIssuesFromServer(workspaceSlug, projectId, queries);
}
Expand All @@ -289,7 +301,15 @@ export class Storage {
const query = issueFilterQueryConstructor(this.workspaceSlug, projectId, queries);
const countQuery = issueFilterCountQueryConstructor(this.workspaceSlug, projectId, queries);
const start = performance.now();
const [issuesRaw, count] = await Promise.all([runQuery(query), runQuery(countQuery)]);
let issuesRaw: any[] = [];
let count: any[];
try {
[issuesRaw, count] = await Promise.all([runQuery(query), runQuery(countQuery)]);
} catch (e) {
logError(e);
const issueService = new IssueService();
return await issueService.getIssuesFromServer(workspaceSlug, projectId, queries);
}
Comment on lines +304 to +312
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 30, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance user feedback when falling back to server due to query failure

In the getIssues method:

try {
  [issuesRaw, count] = await Promise.all([runQuery(query), runQuery(countQuery)]);
} catch (e) {
  logError(e);
  const issueService = new IssueService();
  return await issueService.getIssuesFromServer(workspaceSlug, projectId, queries);
}

Recommendation:

  • Provide user feedback when falling back to the server. Inform the user that local data retrieval failed, and the server is being used instead.
  • This can help with debugging and improves transparency.

Example:

logError(e);
+ logInfo('Local query failed, fetching issues from server');
const issueService = new IssueService();
return await issueService.getIssuesFromServer(workspaceSlug, projectId, queries);

// const issuesRaw = await runQuery(query);
const end = performance.now();

Expand Down Expand Up @@ -318,6 +338,8 @@ export class Storage {
issueResults = getGroupedIssueResults(issueResults);
}
}
const groupCount = group_by ? Object.keys(issueResults).length : undefined;
const subGroupCount = sub_group_by ? Object.keys(issueResults[Object.keys(issueResults)[0]]).length : undefined;
const groupingEnd = performance.now();

const times = {
Expand All @@ -341,6 +363,17 @@ export class Storage {
total_pages,
};

const activeSpan = Sentry.getActiveSpan();
activeSpan?.setAttributes({
projectId,
count: total_count,
groupBy: group_by,
subGroupBy: sub_group_by,
queries: queries,
local: true,
groupCount,
subGroupCount,
});
return out;
};

Expand All @@ -353,6 +386,7 @@ export class Storage {
return formatLocalIssue(issues[0]);
}
} catch (err) {
logError(err);
console.warn("unable to fetch issue from local db");
}

Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -330,7 +330,7 @@ const getSingleFilterFields = (queries: any) => {
export const getIssueFieldsFragment = () => {
const { description_html, ...filtered } = issueSchema;
const keys = Object.keys(filtered);
const sql = ` ${keys.map((key, index) => `i.${key}`).join(`,
const sql = ` ${keys.map((key) => `i.${key}`).join(`,
`)}`;
return sql;
};
14 changes: 13 additions & 1 deletion web/core/local-db/utils/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as Sentry from "@sentry/nextjs";
import pick from "lodash/pick";
import { TIssue } from "@plane/types";
import { rootStore } from "@/lib/store-context";
Expand All @@ -9,7 +10,13 @@ export const log = (...args: any) => {
console.log(...args);
}
};
export const logError = console.error;
export const logError = (e: any) => {
if (e?.result?.errorClass === "SQLite3Error") {
e = parseSQLite3Error(e);
}
Sentry.captureException(e);
console.log(e);
};
export const logInfo = console.info;

export const updatePersistentLayer = async (issueIds: string | string[]) => {
Expand Down Expand Up @@ -140,3 +147,8 @@ export const getSubGroupedIssueResults = (
};

export const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));

const parseSQLite3Error = (error: any) => {
error.result = JSON.stringify(error.result);
return error;
};
Comment on lines +151 to +154
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 30, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve error parsing and handling in parseSQLite3Error

While the parseSQLite3Error function serves its purpose of converting the error result to a JSON string, there are a few improvements that could make it more robust:

  1. Avoid mutating the original error object.
  2. Handle potential JSON.stringify errors.
  3. Preserve the original error information.

Consider refactoring the function as follows:

const parseSQLite3Error = (error: any): any => {
  try {
    const parsedError = {
      ...error,
      result: typeof error.result === 'object' ? JSON.stringify(error.result) : error.result
    };
    return parsedError;
  } catch (jsonError) {
    console.warn('Failed to stringify SQLite3Error result:', jsonError);
    return {
      ...error,
      result: 'Failed to stringify error result'
    };
  }
};

This version:

  • Creates a new error object instead of mutating the original.
  • Only stringifies the result if it's an object.
  • Handles potential JSON.stringify errors.
  • Preserves all original error properties.

6 changes: 5 additions & 1 deletion web/core/services/issue/issue.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as Sentry from "@sentry/nextjs";
// types
import type {
IIssueDisplayProperties,
Expand Down Expand Up @@ -69,7 +70,10 @@ export class IssueService extends APIService {
}

async getIssues(workspaceSlug: string, projectId: string, queries?: any, config = {}): Promise<TIssuesResponse> {
const response = await persistence.getIssues(workspaceSlug, projectId, queries, config);
const response = await Sentry.startSpan({ name: "GET_ISSUES" }, async () => {
const res = await persistence.getIssues(workspaceSlug, projectId, queries, config);
return res;
});
return response as TIssuesResponse;
}

Expand Down