Skip to content

Comments

[WEB-1116] feat: pages realtime sync#5057

Merged
SatishGandham merged 27 commits intodevelopfrom
feat-realtime-sync
Jul 22, 2024
Merged

[WEB-1116] feat: pages realtime sync#5057
SatishGandham merged 27 commits intodevelopfrom
feat-realtime-sync

Conversation

@aaryan610
Copy link
Member

@aaryan610 aaryan610 commented Jul 5, 2024

Realtime sync for Pages

Live server:

Created a new NodeJS server to handle web socket connection using Hocuspocus.

Editor refactoring:

  1. New collaboration hooks-
    • useCollaborationEditor- for editable collaborative editors.
    • useCollaborationReadOnlyEditor- for read only collaborative editors.
  2. Removed old custom CollaborationProvider.

Summary by CodeRabbit

  • New Features

    • Enhanced error handling across multiple endpoints using centralized error codes for improved clarity and maintainability.
    • Introduced new collaborative editing capabilities with the addition of components and hooks for real-time document editing.
    • Added functionalities for managing page descriptions in a collaborative environment.
    • Implemented robust authentication for collaborative document editing, ensuring secure access.
  • Bug Fixes

    • Improved response structure for API errors, providing detailed error messages and codes to aid in debugging.
  • Documentation

    • Updated various configuration files to reflect recent changes and improve the setup process for developers.
  • Chores

    • Added TypeScript configuration and bundler setup files to streamline the development process.

@aaryan610 aaryan610 marked this pull request as ready for review July 15, 2024 09:12
@aaryan610 aaryan610 marked this pull request as draft July 16, 2024 09:04
@sriramveeraghanta sriramveeraghanta changed the base branch from preview to develop July 16, 2024 09:13
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 0d9b346 and b354547.

Files selected for processing (6)
  • apiserver/plane/app/views/page/base.py (3 hunks)
  • live/src/index.ts (1 hunks)
  • live/src/page.ts (1 hunks)
  • packages/editor/src/core/extensions/core-without-props.tsx (4 hunks)
  • packages/editor/src/core/hooks/use-collaborative-editor.ts (3 hunks)
  • packages/editor/src/core/hooks/use-editor.ts (5 hunks)
Files skipped from review as they are similar to previous changes (2)
  • apiserver/plane/app/views/page/base.py
  • live/src/page.ts
Additional context used
Biome
live/src/index.ts

[error] 57-66: Promise executor functions should not be async.

(lint/suspicious/noAsyncPromiseExecutor)


[error] 82-86: Promise executor functions should not be async.

(lint/suspicious/noAsyncPromiseExecutor)

Additional comments not posted (12)
packages/editor/src/core/extensions/core-without-props.tsx (3)

9-17: LGTM!

The new import statements for various custom extensions are correct and necessary for the changes made in the file.


86-86: LGTM!

The new function DocumentEditorExtensionsWithoutProps is correctly implemented.


Line range hint 19-84: Ensure the removal of old extensions is intentional.

The new configuration of CoreEditorExtensionsWithoutProps looks good. However, verify that the removal of CustomKeymap, CustomTypographyExtension, and CustomCodeBlockExtension is intentional and does not affect the functionality.

Verification successful

The removal of old extensions appears to be intentional and does not affect the functionality.

  • No usage of CustomKeymap, CustomTypographyExtension, and CustomCodeBlockExtension was found in the codebase.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the removal of old extensions is intentional and does not affect the functionality.

# Test: Search for the old extensions in the codebase. Expect: No usage of the removed extensions.
rg --type javascript 'CustomKeymap|CustomTypographyExtension|CustomCodeBlockExtension'

Length of output: 123


Script:

#!/bin/bash
# Description: Verify the removal of old extensions is intentional and does not affect the functionality.

# Test: Search for the old extensions in the codebase. Expect: No usage of the removed extensions.
rg --type js 'CustomKeymap|CustomTypographyExtension|CustomCodeBlockExtension'

Length of output: 80

live/src/index.ts (2)

1-11: LGTM!

The new import statements for various modules required for the server setup are correct and necessary for the changes made in the file.


13-40: Ensure proper error handling in authentication.

The server configuration looks good. However, ensure that proper error handling is in place for the authentication process to avoid potential security issues.

Verification successful

Proper error handling is in place in the authentication process.

The handleAuthentication function includes appropriate error handling mechanisms, ensuring that various failure scenarios are addressed correctly.

  • Token mismatch: throw Error("Token doesn't match");
  • Incomplete query params: throw Error("Incomplete query params, workspaceSlug or projectId missing");
  • Invalid document type: throw Error("Invalid document type provided");
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify proper error handling in the authentication process.

# Test: Search for error handling in the authentication process. Expect: Proper error handling in place.
rg --type javascript 'handleAuthentication'

Length of output: 78


Script:

#!/bin/bash
# Description: Verify proper error handling in the authentication process.

# Test: Search for error handling in the authentication process. Expect: Proper error handling in place.
rg --type typescript 'handleAuthentication'

Length of output: 78


Script:

#!/bin/bash
# Description: Verify proper error handling in the authentication process.

# Test: Search for error handling in the authentication process. Expect: Proper error handling in place.
rg 'handleAuthentication' --glob '*.ts'

Length of output: 259


Script:

#!/bin/bash
# Description: Extract and inspect the handleAuthentication function for proper error handling.

# Extract the handleAuthentication function from live/src/authentication.ts
ast-grep --lang typescript --pattern $'export const handleAuthentication = async ($_: Props) => {
  $$$
}' live/src/authentication.ts

Length of output: 2575

packages/editor/src/core/hooks/use-collaborative-editor.ts (3)

1-16: LGTM!

The new import statements for various modules required for the collaborative editor are correct and necessary for the changes made in the file.


Line range hint 18-36: LGTM!

The new type definition CollaborativeEditorProps is correct and does not introduce any issues.


Line range hint 38-107: Ensure proper cleanup of resources.

The new implementation of useCollaborativeEditor looks good. However, ensure that proper cleanup of resources is in place to avoid potential memory leaks.

Verification successful

Proper cleanup of resources confirmed.

The useCollaborativeEditor function includes appropriate cleanup logic within the useEffect and useLayoutEffect hooks to ensure that resources are properly managed and potential memory leaks are avoided.

  • The useEffect hook destroys and disconnects the provider on unmount.
  • The useLayoutEffect hook destroys the localProvider on unmount.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify proper cleanup of resources in the useCollaborativeEditor function.

# Test: Search for resource cleanup in the function. Expect: Proper cleanup of resources in place.
rg --type javascript 'useCollaborativeEditor'

Length of output: 80


Script:

#!/bin/bash
# Description: Verify proper cleanup of resources in the useCollaborativeEditor function.

# Search for useEffect and useLayoutEffect hooks in the specific file to check for cleanup logic.
rg 'useEffect|useLayoutEffect' packages/editor/src/core/hooks/use-collaborative-editor.ts

Length of output: 191


Script:

#!/bin/bash
# Description: Extract the full content of useEffect and useLayoutEffect hooks to review the cleanup logic.

# Extract the content of useEffect and useLayoutEffect hooks.
rg -A 10 'useEffect|useLayoutEffect' packages/editor/src/core/hooks/use-collaborative-editor.ts

Length of output: 997

packages/editor/src/core/hooks/use-editor.ts (4)

62-66: LGTM!

The adjustments in the organization of state and refs within the useEditor hook improve the clarity of state management.


Line range hint 88-104:
LGTM!

The adjustments in the useEffect hook for syncing SWR data improve the handling of the value prop, ensuring that the editor content is synced correctly with the external value.


Line range hint 32-48:
Verify the impact of removing the provider property.

The removal of the provider property from the CustomEditorProps interface simplifies the interface but might affect other parts of the codebase that relied on this property.


Line range hint 50-66:
Verify the impact of removing the provider parameter.

The removal of the provider parameter from the useEditor hook simplifies the hook but might affect other parts of the codebase that relied on this parameter.

Verification successful

Verify the impact of removing the provider parameter.

The removal of the provider parameter from the useEditor hook does not impact the rest of the codebase, as there are no remaining references to this parameter.

  • No occurrences of useEditor with provider parameter were found.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of removing the `provider` parameter from the `useEditor` hook.

# Test: Search for the usage of the `useEditor` hook with the `provider` parameter. Expect: No occurrences of `useEditor` with `provider` parameter.
rg --type typescript --type tsx --type javascript --type jsx -A 5 $'useEditor({ provider:'

Length of output: 198


Script:

#!/bin/bash
# Description: Verify the impact of removing the `provider` parameter from the `useEditor` hook.

# Test 1: Search for the `useEditor` function definition to confirm the removal of the `provider` parameter.
rg --type typescript --type tsx --type javascript --type jsx 'useEditor' -A 10

# Test 2: Search for the usage of the `useEditor` hook with the `provider` parameter.
rg --type typescript --type tsx --type javascript --type jsx -A 5 'useEditor\(\{[^}]*provider:'

Length of output: 246


Script:

#!/bin/bash
# Description: Verify the impact of removing the `provider` parameter from the `useEditor` hook.

# Test 1: Search for the `useEditor` function definition to confirm the removal of the `provider` parameter.
rg --type ts --type tsx --type js --type jsx 'useEditor' -A 10

# Test 2: Search for the usage of the `useEditor` hook with the `provider` parameter.
rg --type ts --type tsx --type js --type jsx -A 5 'useEditor\(\{[^}]*provider:'

Length of output: 200


Script:

#!/bin/bash
# Description: Verify the impact of removing the `provider` parameter from the `useEditor` hook.

# Find all relevant files
fd -e ts -e tsx -e js -e jsx > files.txt

# Test 1: Search for the `useEditor` function definition to confirm the removal of the `provider` parameter.
rg 'useEditor' -A 10 -f files.txt

# Test 2: Search for the usage of the `useEditor` hook with the `provider` parameter.
rg -A 5 'useEditor\(\{[^}]*provider:' -f files.txt

Length of output: 235


Script:

#!/bin/bash
# Description: Verify the impact of removing the `provider` parameter from the `useEditor` hook.

# Test 1: Search for the `useEditor` function definition to confirm the removal of the `provider` parameter.
fd -e ts -e tsx -e js -e jsx | xargs rg 'useEditor' -A 10

# Test 2: Search for the usage of the `useEditor` hook with the `provider` parameter.
fd -e ts -e tsx -e js -e jsx | xargs rg -A 5 'useEditor\(\{[^}]*provider:'

Length of output: 10069

Comment on lines +41 to +87
extensions: [
new Logger(),
new Database({
fetch: async ({
documentName: pageId,
requestHeaders,
requestParameters,
}) => {
// request headers
const cookie = requestHeaders.cookie?.toString();
// query params
const params = requestParameters;
const documentType = params.get("documentType")?.toString() as
| TDocumentTypes
| undefined;

return new Promise(async (resolve) => {
if (documentType === "project_page") {
const fetchedData = await fetchPageDescriptionBinary(
params,
pageId,
cookie,
);
resolve(fetchedData);
}
});
},
store: async ({
state,
documentName: pageId,
requestHeaders,
requestParameters,
}) => {
// request headers
const cookie = requestHeaders.cookie?.toString();
// query params
const params = requestParameters;
const documentType = params.get("documentType")?.toString() as
| TDocumentTypes
| undefined;

return new Promise(async () => {
if (documentType === "project_page") {
await updatePageDescription(params, pageId, state, cookie);
}
});
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid using async functions inside Promise executor functions.

Using async functions inside Promise executor functions can lead to unexpected behavior. Refactor the code to avoid this pattern.

-        return new Promise(async (resolve) => {
+        return new Promise((resolve) => {
           (async () => {
             if (documentType === "project_page") {
               const fetchedData = await fetchPageDescriptionBinary(
                 params,
                 pageId,
                 cookie,
               );
               resolve(fetchedData);
             }
           })();
         });

-        return new Promise(async () => {
+        return new Promise((resolve) => {
           (async () => {
             if (documentType === "project_page") {
               await updatePageDescription(params, pageId, state, cookie);
               resolve();
             }
           })();
         });
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
extensions: [
new Logger(),
new Database({
fetch: async ({
documentName: pageId,
requestHeaders,
requestParameters,
}) => {
// request headers
const cookie = requestHeaders.cookie?.toString();
// query params
const params = requestParameters;
const documentType = params.get("documentType")?.toString() as
| TDocumentTypes
| undefined;
return new Promise(async (resolve) => {
if (documentType === "project_page") {
const fetchedData = await fetchPageDescriptionBinary(
params,
pageId,
cookie,
);
resolve(fetchedData);
}
});
},
store: async ({
state,
documentName: pageId,
requestHeaders,
requestParameters,
}) => {
// request headers
const cookie = requestHeaders.cookie?.toString();
// query params
const params = requestParameters;
const documentType = params.get("documentType")?.toString() as
| TDocumentTypes
| undefined;
return new Promise(async () => {
if (documentType === "project_page") {
await updatePageDescription(params, pageId, state, cookie);
}
});
},
return new Promise((resolve) => {
(async () => {
if (documentType === "project_page") {
const fetchedData = await fetchPageDescriptionBinary(
params,
pageId,
cookie,
);
resolve(fetchedData);
}
})();
});
return new Promise((resolve) => {
(async () => {
if (documentType === "project_page") {
await updatePageDescription(params, pageId, state, cookie);
resolve();
}
})();
});
Tools
Biome

[error] 57-66: Promise executor functions should not be async.

(lint/suspicious/noAsyncPromiseExecutor)


[error] 82-86: Promise executor functions should not be async.

(lint/suspicious/noAsyncPromiseExecutor)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between b354547 and f38fd54.

Files selected for processing (3)
  • live/src/authentication.ts (1 hunks)
  • live/src/page.ts (1 hunks)
  • web/core/components/pages/editor/page-root.tsx (4 hunks)
Files skipped from review as they are similar to previous changes (3)
  • live/src/authentication.ts
  • live/src/page.ts
  • web/core/components/pages/editor/page-root.tsx

@aaryan610 aaryan610 requested a review from SatishGandham July 19, 2024 18:27
@aaryan610 aaryan610 changed the base branch from preview to develop July 22, 2024 09:54
@SatishGandham SatishGandham merged commit 2ea5077 into develop Jul 22, 2024
@SatishGandham SatishGandham deleted the feat-realtime-sync branch July 22, 2024 13:13
sriramveeraghanta added a commit that referenced this pull request Jul 26, 2024
* init: live server for editor realtime sync

* chore: authentication added

* chore: updated logic to convert html to binary for old pages

* chore: added description json on page update

* chore: made all functions generic

* chore: save description in json and html formats

* refactor: document editor components

* chore: uncomment ui package components

* fix: without props extensions refactor

* fix: merge conflicts resolved from preview

* chore: init docker compose

* chore: pages custom error codes

* chore: add health check endpoint to the live server

* chore: update without props extensions type

* chore: better error handling

* chore: update react-hook-form versions

---------

Co-authored-by: NarayanBavisetti <narayan3119@gmail.com>
Co-authored-by: sriram veeraghanta <veeraghanta.sriram@gmail.com>
sriramveeraghanta added a commit that referenced this pull request Sep 2, 2024
* [WEB-1116] feat: pages realtime sync (#5057)

* init: live server for editor realtime sync

* chore: authentication added

* chore: updated logic to convert html to binary for old pages

* chore: added description json on page update

* chore: made all functions generic

* chore: save description in json and html formats

* refactor: document editor components

* chore: uncomment ui package components

* fix: without props extensions refactor

* fix: merge conflicts resolved from preview

* chore: init docker compose

* chore: pages custom error codes

* chore: add health check endpoint to the live server

* chore: update without props extensions type

* chore: better error handling

* chore: update react-hook-form versions

---------

Co-authored-by: NarayanBavisetti <narayan3119@gmail.com>
Co-authored-by: sriram veeraghanta <veeraghanta.sriram@gmail.com>

* fix: docker related fixes

* fix: module type fixes

* fix: nginx update

* fix: adding live server workflow

* fix: workflow fixes

* fix: docker compose fixes

* fix: workflow fixes

* fix: path config

* fix: docker compose warnings

* fix: nginx port forwarding

* fix: update docker compose with new env

* fix: env var fixes

* fix: error handling

* fix: docker compose env var

* fix: compose fixes

* chore: update server start message

* chore: handle errors

* fix: build errors

* chore: update port

* chore: update server port

* chore: show error on authentication fail

* chore: show error on authentication fail

* feat: add redis extension

* chore: updated restore version logic

---------

Co-authored-by: NarayanBavisetti <narayan3119@gmail.com>
Co-authored-by: sriram veeraghanta <veeraghanta.sriram@gmail.com>
Co-authored-by: Palanikannan M <akashmalinimurugu@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants