Skip to content
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.

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

Add YorkieProvider, DocumentProvider and suspense hooks #946

Merged
merged 2 commits into from
Mar 6, 2025

Conversation

hackerwins
Copy link
Member

@hackerwins hackerwins commented Mar 5, 2025

What this PR does / why we need it?

Add YorkieProvider, DocumentProvider and suspense hooks

YorkieProvider and DocumentProvider are React context providers that enable
easy integration of Yorkie clients and documents into your React applications.
They provide convenient hooks for accessing and updating shared documents
in real-time.

<YorkieProvider apiKey={API_KEY}>
  <DocumentProvider
    docKey={DOC_KEY}
    initialRoot={{}}
  >
    <App />
  </DocumentProvider>
</YorkieProvider>

// ...

export default function App() {
  const { root, update, loading, error } = useDocument();

  if (loading) return <div>Loading...</div>;
  if (error) return <div>Error: {error.message}</div>;

// ...
}

Any background context you want to provide?

What are the relevant tickets?

Fixes #

Checklist

  • Added relevant tests or not required
  • Didn't break anything

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Introduced real-time synchronization that enhances document management and collaboration within the to-do application.
    • Added new YorkieProvider and DocumentProvider components for improved state management.
    • Implemented a custom hook useYorkieDoc for better document management.
  • Refactor

    • Streamlined component structure and simplified loading/error handling for an improved user experience.
    • Updated function signatures and prop destructuring for better code clarity.
    • Renamed the useDocument hook to useYorkieDoc to better reflect its purpose.
  • Chores

    • Updated development configuration to support the latest React capabilities, including JSX transform and Vite plugin integration.
    • Enhanced module exports to include the Presence entity for improved functionality.

Copy link

coderabbitai bot commented Mar 5, 2025

Warning

Rate limit exceeded

@hackerwins has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 6 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between a055289 and 469b9a6.

📒 Files selected for processing (5)
  • examples/react-todomvc/src/App.tsx (1 hunks)
  • packages/react/src/DocumentProvider.tsx (1 hunks)
  • packages/react/src/YorkieProvider.tsx (1 hunks)
  • packages/react/src/index.ts (1 hunks)
  • packages/sdk/src/yorkie.ts (1 hunks)

Walkthrough

This pull request refactors several parts of the codebase by simplifying hook usage and component structure in the React TodoMVC example. It removes unnecessary configurations, standardizes variable names, and integrates new Yorkie SDK–based providers and hooks. New components for document and client context management have been added, and the exports are updated accordingly. Additionally, minor configuration updates in TypeScript and Vite strengthen React support.

Changes

File(s) Change Summary
examples/react-todomvc/App.tsx
examples/react-todomvc/MainSection.tsx
examples/react-todomvc/main.tsx
Simplified useDocument hook usage (removed parameters), streamlined error/loading checks, standardized variable naming, refactored function signature via destructuring, and integrated YorkieProvider and DocumentProvider around the App component.
packages/react/src/DocumentProvider.tsx
packages/react/src/YorkieProvider.tsx
packages/react/src/useYorkieDoc.ts
packages/react/src/index.ts
Introduced new context providers and custom hooks for Yorkie client and document management; renamed useDocument to useYorkieDoc for clarity; updated module exports to include the new functionalities.
packages/react/tsconfig.json
packages/react/vite.config.js
Updated TypeScript configuration with "jsx": "react-jsx" and added the React plugin in Vite to support React features during development and build time.

Possibly related PRs

Suggested labels

enhancement 🌟

Suggested reviewers

  • chacha912

Poem

I'm a rabbit in a coding land,
Hopping through files with a nimble hand.
Doc and Yorkie now lead the play,
Simplifying our work day by day.
With clean hooks and a joyful beat,
Our code races—oh so neat! 🐰✨


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@hackerwins hackerwins changed the title Add YorkieProvider and DocumentProvider Add YorkieProvider and DocumentProvider and its suspense hooks Mar 5, 2025
@hackerwins hackerwins changed the title Add YorkieProvider and DocumentProvider and its suspense hooks Add YorkieProvider, DocumentProvider and suspense hooks Mar 5, 2025
@hackerwins hackerwins marked this pull request as draft March 5, 2025 10:19
Copy link

@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: 3

🧹 Nitpick comments (11)
examples/react-todomvc/src/main.tsx (2)

5-11: Fix the typo in variable name.

There's a typo in the variable name: initalRoot should be initialRoot.

-const initalRoot = {
+const initialRoot = {
  todos: [
    { id: 0, text: 'Yorkie JS SDK', completed: false },
    { id: 1, text: 'Garbage collection', completed: false },
    { id: 2, text: 'RichText datatype', completed: false },
  ],
};

14-21: Check for environment variable existence.

The API key is directly accessed without checking if it exists, which could lead to runtime errors if the environment variable is not defined.

Consider adding a fallback or showing a meaningful error:

-  <YorkieProvider apiKey={import.meta.env.VITE_YORKIE_API_KEY}>
+  <YorkieProvider apiKey={import.meta.env.VITE_YORKIE_API_KEY || ''}>

Additionally, you might want to consider if creating a new document each day (based on the date in the docKey) is the intended behavior.

Does the application need to create a new document every day, or should it maintain the same document across days?

examples/react-todomvc/src/App.tsx (1)

32-43: Consider optimizing todo lookup.

The current implementation iterates through all todos to find a matching id. This could be optimized using methods like find or by maintaining a map of todos by id.

deleteTodo: (id: number) => {
  update((root) => {
-    let target: (Todo & JSONObject<Todo>) | undefined;
-    for (const todo of root.todos) {
-      if (todo.id === id) {
-        target = todo as Todo & JSONObject<Todo>;
-        break;
-      }
-    }
+    const target = root.todos.find(todo => todo.id === id) as (Todo & JSONObject<Todo>) | undefined;
    if (target) {
      root.todos.deleteByID!(target.getID!());
    }
  });
},

This pattern could be applied to other similar blocks in the file (editTodo, completeTodo).

packages/react/src/YorkieProvider.tsx (3)

1-3: Check copyright year

The copyright notice shows 2025, which is in the future. This should be updated to the current year or the year when the code was created.

-/*
- * Copyright 2025 The Yorkie Authors. All rights reserved.
+/*
+ * Copyright 2024 The Yorkie Authors. All rights reserved.

24-24: Consider importing from package root instead of src directory

Importing directly from src folder is generally not recommended as it bypasses the package's public API and may lead to issues if the internal structure changes.

-import { Client } from '@yorkie-js-sdk/src/yorkie';
+import { Client } from '@yorkie-js/sdk';

68-71: Improve error handling in useYorkie hook

The useYorkie hook should check if the context exists before accessing the client property to prevent potential runtime errors.

export const useYorkie = () => {
  const context = useContext(YorkieContext);
+  if (!context) {
+    throw new Error('useYorkie must be used within a YorkieProvider');
+  }
  return context?.client;
};
packages/react/src/DocumentProvider.tsx (4)

1-3: Check copyright year

The copyright notice shows 2025, which is in the future. This should be updated to the current year or the year when the code was created.

-/*
- * Copyright 2025 The Yorkie Authors. All rights reserved.
+/*
+ * Copyright 2024 The Yorkie Authors. All rights reserved.

21-27: Consider using more specific types instead of 'any'

Using any type loses TypeScript's benefits. Consider using more specific types for root and presence.

const DocumentContext = createContext<{
-  root: any;
-  presences: { clientID: string; presence: any }[];
+  root: Indexable;
+  presences: { clientID: string; presence: Record<string, unknown> }[];
  update: (callback: (root: any) => void) => void;
  loading: boolean;
  error: Error | undefined;
} | null>(null);

80-84: Add loading and error state to the update function

The update function should handle potential errors and update loading state to provide better feedback to users.

const update = (callback: (root: any) => void) => {
  if (doc) {
-    doc.update(callback);
+    try {
+      setLoading(true);
+      doc.update(callback);
+    } catch (err) {
+      setError(err instanceof Error ? err : new Error('Failed to update document'));
+    } finally {
+      setLoading(false);
+    }
  }
};

125-134: Complete documentation for usePresence hook

The usePresence hook has incomplete documentation compared to other hooks.

/**
 * `usePresence` is a custom hook that returns the presence of the document.
+ * This hook must be used within a `DocumentProvider`.
 */
export const usePresence = () => {
  const context = useContext(DocumentContext);
  if (!context) {
    throw new Error('usePresence must be used within a DocumentProvider');
  }
  return context.presences;
};
packages/react/src/index.ts (1)

29-36: Consider adding JSDoc comments for exported entities

To improve developer experience, consider adding JSDoc comments for the exported entities that briefly explain their purpose.

export type { JSONArray, JSONObject };
+/**
+ * Exports components and hooks for Yorkie integration with React
+ * - YorkieProvider: Provider for Yorkie client
+ * - DocumentProvider: Provider for Yorkie document
+ * - useDocument: Hook to access document data
+ * - useRoot: Hook to access document root
+ * - usePresence: Hook to access document presences
+ * - useYorkieDoc: Legacy hook for Yorkie document
+ */
export {
  YorkieProvider,
  DocumentProvider,
  useDocument,
  useRoot,
  usePresence,
  useYorkieDoc,
};
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1111636 and 96a75b5.

📒 Files selected for processing (9)
  • examples/react-todomvc/src/App.tsx (1 hunks)
  • examples/react-todomvc/src/MainSection.tsx (2 hunks)
  • examples/react-todomvc/src/main.tsx (1 hunks)
  • packages/react/src/DocumentProvider.tsx (1 hunks)
  • packages/react/src/YorkieProvider.tsx (1 hunks)
  • packages/react/src/index.ts (1 hunks)
  • packages/react/src/useYorkieDoc.ts (1 hunks)
  • packages/react/tsconfig.json (1 hunks)
  • packages/react/vite.config.js (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • packages/react/src/useYorkieDoc.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build (18.x)
🔇 Additional comments (10)
packages/react/tsconfig.json (1)

9-9: Good addition of React JSX support!

Adding "jsx": "react-jsx" to the TypeScript configuration is essential for the new React components being introduced. This enables the newer JSX transform from React 17+ which is more efficient and doesn't require importing React in every JSX file.

packages/react/vite.config.js (1)

3-3: Appropriate addition of the React Vite plugin.

Adding the React plugin is necessary for proper React support in the Vite build process. This enables React-specific optimizations, Fast Refresh during development, and proper JSX handling.

Also applies to: 28-28

examples/react-todomvc/src/main.tsx (1)

3-3: Good import of the new provider components.

The import of DocumentProvider and YorkieProvider aligns with the PR objectives to introduce these new components.

examples/react-todomvc/src/App.tsx (4)

1-1: Clean import statement from the React package.

The import now correctly references the components from the '@yorkie-js/react' package, which aligns with the introduction of the new providers.


14-14: Simplified useDocument hook usage.

The hook call has been simplified by removing the explicit configuration, as it's now handled by the providers in main.tsx. This follows good React practices by moving configuration to higher-level components.


16-17: Improved conditional rendering.

The loading and error states now use concise return statements instead of block statements, which is a more modern and readable React pattern.


24-24: Consistent variable naming.

Changed from maxId to maxID, which provides better consistency in the codebase.

examples/react-todomvc/src/MainSection.tsx (2)

19-19: LGTM: Good use of parameter destructuring

The refactoring to use destructuring in the function parameters improves code readability.


39-47: LGTM: Improved JSX formatting

The JSX formatting changes make the code more concise while maintaining readability.

packages/react/src/index.ts (1)

19-26: LGTM: Well-organized imports

The new import structure clearly organizes all the necessary components and hooks.

Copy link

codecov bot commented Mar 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.50%. Comparing base (1111636) to head (469b9a6).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #946      +/-   ##
==========================================
- Coverage   78.54%   78.50%   -0.04%     
==========================================
  Files          63       63              
  Lines        5421     5421              
  Branches      998      998              
==========================================
- Hits         4258     4256       -2     
  Misses        874      874              
- Partials      289      291       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hackerwins hackerwins marked this pull request as ready for review March 5, 2025 10:59
Copy link

@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: 3

🧹 Nitpick comments (5)
examples/react-todomvc/src/main.tsx (2)

5-5: Fix typo in variable name initalRoot

The variable name has a spelling error - it should be initialRoot instead of initalRoot (missing an "i").

-const initalRoot = {
+const initialRoot = {

14-21: Consider more unique docKey generation for multi-user scenarios

The current docKey is based only on the date, which could lead to collisions if multiple users access the application on the same day.

-      docKey={`react-todomvc-${new Date().toISOString().substring(0, 10).replace(/-/g, '')}`}
+      docKey={`react-todomvc-${new Date().toISOString().substring(0, 10).replace(/-/g, '')}-${Math.random().toString(36).substring(2, 9)}`}
packages/react/src/DocumentProvider.tsx (3)

51-58: Consider adding a timeout or retry mechanism for document attachment

The document attachment logic could benefit from a timeout or retry mechanism to handle network issues or service unavailability.

useEffect(() => {
  setLoading(true);
  setError(undefined);

  if (!client) return;

  const newDoc = new Document<R, P>(docKey);
+  let retryCount = 0;
+  const maxRetries = 3;

  async function attachDocument() {
+    if (retryCount >= maxRetries) {
+      setError(new Error(`Failed to attach document after ${maxRetries} attempts`));
+      setLoading(false);
+      return;
+    }

135-137: Complete the JSDoc comment for usePresence

The JSDoc comment for usePresence is incomplete. It should mention that the hook must be used within a DocumentProvider like the other hook comments do.

/**
- * `usePresence` is a custom hook that returns the presence of the document.
+ * `usePresence` is a custom hook that returns the presence of the document.
+ * This hook must be used within a `DocumentProvider`.
 */

57-86: Consider using a more robust document attachment pattern

The document attachment logic could be improved by separating the subscription setup into its own function and adding more robust error handling.

const newDoc = new Document<R, P>(docKey);
async function attachDocument() {
  try {
    await client?.attach(newDoc);
-
-    newDoc.subscribe(() => {
-      setRoot({ ...newDoc.getRoot() });
-    });
-
-    newDoc.subscribe('presence', () => {
-      setPresences(newDoc.getPresences());
-    });
+    setupSubscriptions(newDoc);
    
    setDoc(newDoc);
    setRoot({ ...newDoc.getRoot() });
  } catch (err) {
    setError(
      err instanceof Error ? err : new Error('Failed to attach document'),
    );
  } finally {
    setLoading(false);
  }
}

+function setupSubscriptions(document: Document<R, P>) {
+  document.subscribe(() => {
+    setRoot({ ...document.getRoot() });
+  });
+
+  document.subscribe('presence', () => {
+    setPresences(document.getPresences());
+  });
+}

attachDocument();

return () => {
-  client.detach(newDoc);
+  if (client && newDoc) {
+    try {
+      client.detach(newDoc);
+    } catch (err) {
+      console.error('Failed to detach document:', err);
+    }
+  }
};
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 96a75b5 and 1e37976.

📒 Files selected for processing (9)
  • examples/react-todomvc/src/App.tsx (1 hunks)
  • examples/react-todomvc/src/MainSection.tsx (2 hunks)
  • examples/react-todomvc/src/main.tsx (1 hunks)
  • packages/react/src/DocumentProvider.tsx (1 hunks)
  • packages/react/src/YorkieProvider.tsx (1 hunks)
  • packages/react/src/index.ts (1 hunks)
  • packages/react/src/useYorkieDoc.ts (1 hunks)
  • packages/react/tsconfig.json (1 hunks)
  • packages/react/vite.config.js (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
  • packages/react/vite.config.js
  • packages/react/tsconfig.json
  • packages/react/src/useYorkieDoc.ts
  • packages/react/src/YorkieProvider.tsx
  • examples/react-todomvc/src/App.tsx
  • examples/react-todomvc/src/MainSection.tsx
🔇 Additional comments (7)
examples/react-todomvc/src/main.tsx (1)

5-11: The default todos look good

The initial todo items provide a clear example of how the todos should be structured, with appropriate fields (id, text, completed).

packages/react/src/index.ts (2)

17-28: Organized imports look clean

The structure of imports is well-organized, clearly separating the core types from the custom hooks and providers.


29-36: Well-structured exports provide a complete public API

The exports include all necessary components (YorkieProvider, DocumentProvider) and hooks (useDocument, useRoot, usePresence, useYorkieDoc), making them accessible to consumers of the package.

packages/react/src/DocumentProvider.tsx (4)

18-18: Use consistent import path for Yorkie SDK

There appears to be an inconsistency in import paths across files. Ensure that all files use the same import path for the Yorkie SDK.


21-27: Well-defined context type with appropriate fields

The DocumentContextType is well-structured, including all necessary fields for document management: root, presences, update function, loading state, and error handling.


33-41: Good use of generics for type safety

The DocumentProvider component properly uses TypeScript generics to ensure type safety across the document root and presence objects.


83-85: Add client null check in cleanup function

The cleanup function should check if the client exists before attempting to detach the document.

Copy link

@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: 2

♻️ Duplicate comments (2)
packages/react/src/DocumentProvider.tsx (2)

86-88: ⚠️ Potential issue

Add client null check in cleanup function

The cleanup function should check if the client exists before attempting to detach the document.

return () => {
-  client.detach(newDoc);
+  if (client) {
+    client.detach(newDoc);
+  }
};

119-120: ⚠️ Potential issue

Fix type mismatch in useDocument update function

The update function type defined in useDocument doesn't match the original context definition. The callback in the returned object includes a presence parameter that isn't present in the context type definition.

update: context.update as (
-  callback: (root: R, presence: P) => void,
+  callback: (root: R) => void,
) => void,
🧹 Nitpick comments (3)
packages/react/src/DocumentProvider.tsx (2)

138-147: Complete JSDoc comment for usePresences hook

The JSDoc comment for usePresences is incomplete compared to other hooks. It should include the same information about it being used within a DocumentProvider.

/**
 * `usePresences` is a custom hook that returns the presences of the document.
+ * This hook must be used within a `DocumentProvider`.
 */

65-67: Consider optimizing root object updates

Creating a new object on every update with { ...newDoc.getRoot() } could lead to performance issues if the root object is large. Consider implementing a more efficient update mechanism that only triggers re-renders when necessary.

newDoc.subscribe(() => {
-  setRoot({ ...newDoc.getRoot() });
+  // Only update if there are actual changes
+  const newRoot = newDoc.getRoot();
+  setRoot(prevRoot => {
+    // Simple comparison might not be sufficient for complex objects
+    // Consider using a deep comparison library if needed
+    return JSON.stringify(prevRoot) !== JSON.stringify(newRoot) ? { ...newRoot } : prevRoot;
+  });
});
packages/sdk/src/yorkie.ts (1)

53-53: Ensure Presence is included in default export and globalThis

You've correctly exported the Presence type, but for consistency, consider also adding it to the default export object (lines 142-153) and the globalThis exposure (lines 156-169).

export default {
  Client,
  Document,
  Primitive,
  Text,
  Counter,
  Tree,
+ Presence,
  LogLevel,
  setLogLevel,
  IntType: CounterType.IntegerCnt,
  LongType: CounterType.LongCnt,
};

// And also update the globalThis definition
if (typeof globalThis !== 'undefined') {
  (globalThis as any).yorkie = {
    Client,
    Document,
    Primitive,
    Text,
    Counter,
    Tree,
+   Presence,
    LogLevel,
    setLogLevel,
    IntType: CounterType.IntegerCnt,
    LongType: CounterType.LongCnt,
  };
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1e37976 and 28d783e.

📒 Files selected for processing (5)
  • examples/react-todomvc/src/App.tsx (1 hunks)
  • packages/react/src/DocumentProvider.tsx (1 hunks)
  • packages/react/src/YorkieProvider.tsx (1 hunks)
  • packages/react/src/index.ts (1 hunks)
  • packages/sdk/src/yorkie.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • examples/react-todomvc/src/App.tsx
  • packages/react/src/YorkieProvider.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build (18.x)

Copy link

@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

♻️ Duplicate comments (3)
packages/react/src/DocumentProvider.tsx (3)

86-88: ⚠️ Potential issue

Add client null check in cleanup function

The cleanup function should check if the client exists before attempting to detach the document to prevent potential errors if the component unmounts when the client is null or undefined.

return () => {
-  client.detach(newDoc);
+  if (client) {
+    client.detach(newDoc);
+  }
};

91-95: ⚠️ Potential issue

Fix parameter type mismatch and add error handling to update function

There are two issues with the update function:

  1. The parameter type (callback: (root: R, presence: Presence<P>) => void) doesn't match the type defined in DocumentContextType on line 24 (update: (callback: (root: R) => void) => void).
  2. The function lacks error handling to prevent unhandled exceptions.
-  const update = (callback: (root: R, presence: Presence<P>) => void) => {
+  const update = (callback: (root: R) => void) => {
     if (doc) {
-      doc.update(callback);
+      try {
+        doc.update(callback);
+      } catch (err) {
+        setError(err instanceof Error ? err : new Error('Failed to update document'));
+      }
     }
   };

116-123: ⚠️ Potential issue

Fix type mismatch in useDocument update function

The update function type in the useDocument hook doesn't match what's defined in the DocumentContextType. The callback includes a presence parameter that's not present in the context type definition.

update: context.update as (
-  callback: (root: R, presence: P) => void,
+  callback: (root: R) => void,
) => void,
🧹 Nitpick comments (4)
packages/react/src/DocumentProvider.tsx (3)

138-147: Update usePresences documentation for consistency

The documentation for usePresences is missing the statement that it must be used within a DocumentProvider, unlike the other hooks which include this important information.

/**
 * `usePresences` is a custom hook that returns the presences of the document.
+ * This hook must be used within a `DocumentProvider`.
 */

45-52: Optimize state initialization and add types for better type safety

The state initialization could be more type-safe and explicit to prevent potential type errors.

  const client = useYorkie();
-  const [doc, setDoc] = useState<Document<R, P> | undefined>(undefined);
+  const [doc, setDoc] = useState<Document<R, P> | undefined>();
  const [loading, setLoading] = useState<boolean>(true);
-  const [error, setError] = useState<Error | undefined>(undefined);
+  const [error, setError] = useState<Error | undefined>();
  const [root, setRoot] = useState(initialRoot);
  const [presences, setPresences] = useState<
-    { clientID: string; presence: any }[]
+    { clientID: string; presence: P }[]
  >([]);

97-103: Memoize the context value to prevent unnecessary re-renders

The context value object is recreated on every render, which can cause unnecessary re-renders in consuming components. Consider memoizing it with useMemo.

+ import { createContext, useContext, useEffect, useState, useMemo } from 'react';

  // ...

+  const contextValue = useMemo(
+    () => ({ root, presences, update, loading, error }),
+    [root, presences, update, loading, error]
+  );

  return (
    <DocumentContext.Provider
-      value={{ root, presences, update, loading, error }}
+      value={contextValue}
    >
      {children}
    </DocumentContext.Provider>
  );
packages/react/src/index.ts (1)

28-36: Consider adding proper JSDoc comments for exports

Adding JSDoc comments for the exported components and hooks would improve the developer experience by providing documentation directly in the IDE.

export type { JSONArray, JSONObject };
+
+/**
+ * Provides Yorkie client context to the application
+ */
export {
  YorkieProvider,
+
+/**
+ * Provides document context to manage document state
+ */
  DocumentProvider,
+
+/**
+ * Hook to access document state including root, presences, update function, loading and error states
+ */
  useDocument,
+
+/**
+ * Hook to access only the root object of the document
+ */
  useRoot,
+
+/**
+ * Hook to access only the presences of the document
+ */
  usePresences,
+
+/**
+ * Legacy hook for backward compatibility
+ */
  useYorkieDoc,
};
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 28d783e and a055289.

📒 Files selected for processing (5)
  • examples/react-todomvc/src/App.tsx (1 hunks)
  • packages/react/src/DocumentProvider.tsx (1 hunks)
  • packages/react/src/YorkieProvider.tsx (1 hunks)
  • packages/react/src/index.ts (1 hunks)
  • packages/sdk/src/yorkie.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • examples/react-todomvc/src/App.tsx
  • packages/react/src/YorkieProvider.tsx
  • packages/sdk/src/yorkie.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build (18.x)
🔇 Additional comments (1)
packages/react/src/index.ts (1)

25-26:

✅ Verification successful

Maintain consistent naming between import and export for usePresences

There's a potential naming consistency issue with usePresences (plural form). If this is intended to be used as a singular concept elsewhere in the codebase, consider standardizing the naming to prevent confusion.

Also applies to: 34-35


🏁 Script executed:

#!/bin/bash
# Description: Check how the usePresences hook is used in the codebase to verify naming consistency

# Check for usage of usePresence(s) in the codebase
echo "Checking for usage of usePresence(s) in the codebase:"
rg -i "usePresence" --type ts --type tsx

# Check for patterns related to presence in the codebase
echo -e "\nChecking for presence-related patterns:"
rg -i "presence" --type ts --type tsx | grep -i "function\|hook\|const"

Length of output: 380


Below is a new shell script to re-run the verification using file globs rather than file type identifiers. This should accurately search for both “usePresences” and “usePresence” in all TS/TSX files:

Please run the above script to gather a clearer picture of how these hooks are used across the codebase. Once you’ve confirmed that the naming is consistent (or identified areas where singular versus plural usage might cause confusion), consider one of the following actions:

  • If usage is consistent: Approve keeping usePresences as is.
  • If naming differs (e.g. both singular and plural forms are used): Standardize the naming (either switch to usePresence or update all occurrences to usePresences) to avoid confusion. This applies to the import at lines 25–26 and the similar pattern at lines 34–35 in packages/react/src/index.ts.

Once you verify this manually (or if the re-run script provides the expected output), please update the code as needed.

Action Required: Verify Naming Consistency for usePresences Across TS/TSX Files

  • Confirm consistent usage of usePresences vs. usePresence in the DocumentProvider and elsewhere.
  • Update naming if discrepancies are detected.

🏁 Script executed:

#!/bin/bash
# Re-run search with file globs to verify naming consistency for usePresences
echo "Searching for 'usePresences' occurrences in TS/TSX files:"
rg -i "usePresences" --glob "*.ts" --glob "*.tsx"

echo -e "\nSearching for 'usePresence' occurrences in TS/TSX files:"
rg -i "usePresence" --glob "*.ts" --glob "*.tsx"

echo -e "\nSearching for generic 'presence' patterns (function/hook/const) in TS/TSX files:"
rg -i "presence" --glob "*.ts" --glob "*.tsx" | grep -i -E "function|hook|const"

Length of output: 16600


Naming Consistency Verified – No Changes Needed

After verifying the codebase, the custom hook is consistently exported and imported as usePresences in both packages/react/src/DocumentProvider.tsx and packages/react/src/index.ts (lines 25–26 and 34–35). No occurrences of a singular form (usePresence) were found, so the current naming is consistent across the codebase.

@hackerwins hackerwins merged commit c873cd1 into main Mar 6, 2025
2 checks passed
@hackerwins hackerwins deleted the react-provider branch March 6, 2025 02:51
hackerwins added a commit that referenced this pull request Mar 6, 2025
YorkieProvider and DocumentProvider are React context providers that enable
easy integration of Yorkie clients and documents into React applications.
They also provide convenient hooks for accessing and updating shared documents
in real-time.

```tsx
<YorkieProvider apiKey={API_KEY}>
  <DocumentProvider
    docKey={DOC_KEY}
    initialRoot={{}}
  >
    <App />
  </DocumentProvider>
</YorkieProvider>

// ...

export default function App() {
  const { root, update, loading, error } = useDocument();

  if (loading) return <div>Loading...</div>;
  if (error) return <div>Error: {error.message}</div>;

// ...
}
```
hackerwins added a commit that referenced this pull request Mar 6, 2025
YorkieProvider and DocumentProvider are React context providers that enable
easy integration of Yorkie clients and documents into React applications.
They also provide convenient hooks for accessing and updating shared documents
in real-time.

```tsx
<YorkieProvider apiKey={API_KEY}>
  <DocumentProvider
    docKey={DOC_KEY}
    initialRoot={{}}
  >
    <App />
  </DocumentProvider>
</YorkieProvider>

// ...

export default function App() {
  const { root, update, loading, error } = useDocument();

  if (loading) return <div>Loading...</div>;
  if (error) return <div>Error: {error.message}</div>;

// ...
}
```
hackerwins added a commit that referenced this pull request Mar 6, 2025
YorkieProvider and DocumentProvider are React context providers that enable
easy integration of Yorkie clients and documents into React applications.
They also provide convenient hooks for accessing and updating shared documents
in real-time.

```tsx
<YorkieProvider apiKey={API_KEY}>
  <DocumentProvider
    docKey={DOC_KEY}
    initialRoot={{}}
  >
    <App />
  </DocumentProvider>
</YorkieProvider>

// ...

export default function App() {
  const { root, update, loading, error } = useDocument();

  if (loading) return <div>Loading...</div>;
  if (error) return <div>Error: {error.message}</div>;

// ...
}
```
hackerwins added a commit that referenced this pull request Mar 6, 2025
YorkieProvider and DocumentProvider are React context providers that enable
easy integration of Yorkie clients and documents into React applications.
They also provide convenient hooks for accessing and updating shared documents
in real-time.

```tsx
<YorkieProvider apiKey={API_KEY}>
  <DocumentProvider
    docKey={DOC_KEY}
    initialRoot={{}}
  >
    <App />
  </DocumentProvider>
</YorkieProvider>

// ...

export default function App() {
  const { root, update, loading, error } = useDocument();

  if (loading) return <div>Loading...</div>;
  if (error) return <div>Error: {error.message}</div>;

// ...
}
```
hackerwins added a commit that referenced this pull request Mar 6, 2025
YorkieProvider and DocumentProvider are React context providers that enable
easy integration of Yorkie clients and documents into React applications.
They also provide convenient hooks for accessing and updating shared documents
in real-time.

```tsx
<YorkieProvider apiKey={API_KEY}>
  <DocumentProvider
    docKey={DOC_KEY}
    initialRoot={{}}
  >
    <App />
  </DocumentProvider>
</YorkieProvider>

// ...

export default function App() {
  const { root, update, loading, error } = useDocument();

  if (loading) return <div>Loading...</div>;
  if (error) return <div>Error: {error.message}</div>;

// ...
}
```
hackerwins added a commit that referenced this pull request Mar 6, 2025
YorkieProvider and DocumentProvider are React context providers that enable
easy integration of Yorkie clients and documents into React applications.
They also provide convenient hooks for accessing and updating shared documents
in real-time.

```tsx
<YorkieProvider apiKey={API_KEY}>
  <DocumentProvider
    docKey={DOC_KEY}
    initialRoot={{}}
  >
    <App />
  </DocumentProvider>
</YorkieProvider>

// ...

export default function App() {
  const { root, update, loading, error } = useDocument();

  if (loading) return <div>Loading...</div>;
  if (error) return <div>Error: {error.message}</div>;

// ...
}
```
hackerwins added a commit that referenced this pull request Mar 6, 2025
YorkieProvider and DocumentProvider are React context providers that enable
easy integration of Yorkie clients and documents into React applications.
They also provide convenient hooks for accessing and updating shared documents
in real-time.

```tsx
<YorkieProvider apiKey={API_KEY}>
  <DocumentProvider
    docKey={DOC_KEY}
    initialRoot={{}}
  >
    <App />
  </DocumentProvider>
</YorkieProvider>

// ...

export default function App() {
  const { root, update, loading, error } = useDocument();

  if (loading) return <div>Loading...</div>;
  if (error) return <div>Error: {error.message}</div>;

// ...
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant