Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @pakxe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 Pull Request는 마인드맵 애플리케이션에 실시간 동시 편집 기능을 도입하기 위한 초기 단계의 구현을 포함합니다. Yjs 라이브러리를 활용하여 여러 사용자가 동시에 마인드맵을 수정할 수 있는 기반을 마련했으며, 오프라인 상태에서의 작업 내용도 안전하게 보존하고 동기화할 수 있도록 IndexedDB 연동을 추가했습니다. 이는 사용자 경험을 크게 향상시키고 협업 기능을 강화하는 중요한 변화입니다. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이 PR은 Y.js를 사용하여 마인드맵의 동시 편집 기능을 구현하는 것을 목표로 합니다. 새로운 의존성을 추가하고, 상태 공유를 위한 여러 훅과 유틸리티 클래스(SharedMindmapController, SharedTreeContainer 등)를 도입했습니다. 전반적으로 협업 기능을 위한 좋은 기반을 마련했지만, 몇 가지 중요한 수정이 필요합니다. 특히, 원격 변경 사항에 대한 레이아웃 업데이트 로직, useLayoutEffect의 의존성 배열, 그리고 재귀적 노드 삭제 시의 트랜잭션 처리 방식에서 버그와 성능 문제가 발견되었습니다. 또한 코드 품질과 안정성을 높이기 위한 몇 가지 개선 사항을 제안합니다.
| if (!event.transaction.local) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
원격 변경 사항(!event.transaction.local)에 대해 레이아웃 계산을 건너뛰고 있습니다. 이로 인해 다른 사용자가 노드를 추가하거나 삭제했을 때, 내 화면에서는 레이아웃이 갱신되지 않아 노드가 겹치거나 잘못된 위치에 표시될 수 있습니다. 구조적인 변경(노드 추가, 삭제, 이동 등)에 대해서는 변경의 출처와 상관없이 레이아웃을 다시 계산해야 동시 편집 환경에서 일관된 UI를 보장할 수 있습니다.
| if (!event.transaction.local) { | |
| return; | |
| } | |
| // if (!event.transaction.local) { | |
| // return; | |
| // } | |
| if (isChanged) { | ||
| onResize({ height: newHeight, width: newWidth }); | ||
| } | ||
| }, [nodeId]); |
There was a problem hiding this comment.
useLayoutEffect의 의존성 배열이 [nodeId]로 되어 있습니다. nodeId는 일반적으로 변경되지 않는 값이므로, 이 effect는 컴포넌트가 마운트될 때 한 번만 실행됩니다. 노드의 콘텐츠가 변경되어 크기가 바뀌는 경우를 감지하려면 node 객체 자체를 의존성 배열에 추가해야 합니다. 또한, React Hook의 규칙에 따라 effect 내부에서 사용하는 모든 외부 스코프의 값들(node, onResize)을 의존성 배열에 포함하는 것이 좋습니다.
| }, [nodeId]); | |
| }, [node, onResize]); |
| private _deleteTraverse({ nodeId }: { nodeId: NodeId }) { | ||
| const node = this.safeGetNode(nodeId); | ||
| if (!node) return; | ||
|
|
||
| let childId = node.firstChildId; | ||
|
|
||
| while (childId) { | ||
| const child = this.safeGetNode(childId); | ||
| if (!child) break; | ||
|
|
||
| const nextChildId = child.nextId; | ||
| this._deleteTraverse({ nodeId: childId }); | ||
| childId = nextChildId; | ||
| } | ||
|
|
||
| this.deleteNode(nodeId); | ||
| } |
There was a problem hiding this comment.
_deleteTraverse 메서드는 하위 노드를 삭제하기 위해 deleteNode를 재귀적으로 호출합니다. deleteNode는 각 호출마다 새로운 Y.js 트랜잭션을 생성하므로, 서브트리 전체를 삭제할 때 각 노드마다 별도의 트랜잭션이 발생하게 됩니다. 이는 매우 비효율적이며, 작업의 원자성을 깨뜨려 undo/redo 기능에 문제를 일으킬 수 있습니다. 전체 삭제 과정은 단일 트랜잭션 내에서 처리되어야 합니다.
_deleteTraverse 내부에서 this.deleteNode(nodeId)를 호출하는 대신, 트랜잭션을 생성하지 않는 내부 삭제 로직(예: this.yNodes.delete(nodeId))을 직접 호출하도록 수정하는 것을 권장합니다. 또한, private 헬퍼 메서드의 견고성을 높이고 인자 전달의 명확성을 위해, 인자를 객체 형태로 전달하고 내부에서 유효성 검사를 수행하도록 개선했습니다.
| private _deleteTraverse({ nodeId }: { nodeId: NodeId }) { | |
| const node = this.safeGetNode(nodeId); | |
| if (!node) return; | |
| let childId = node.firstChildId; | |
| while (childId) { | |
| const child = this.safeGetNode(childId); | |
| if (!child) break; | |
| const nextChildId = child.nextId; | |
| this._deleteTraverse({ nodeId: childId }); | |
| childId = nextChildId; | |
| } | |
| this.deleteNode(nodeId); | |
| } | |
| private _deleteTraverse({ nodeId }: { nodeId: NodeId }) { | |
| const node = this.safeGetNode(nodeId); | |
| if (!node) return; | |
| let childId = node.firstChildId; | |
| while (childId) { | |
| const child = this.safeGetNode(childId); | |
| if (!child) break; | |
| const nextChildId = child.nextId; | |
| this._deleteTraverse({ nodeId: childId }); | |
| childId = nextChildId; | |
| } | |
| this.yNodes.delete(nodeId); | |
| this.cachedNodes.delete(nodeId); | |
| } |
References
- Private helper methods should perform their own validation (e.g., null checks) to be robust and self-contained, even if the calling context is expected to perform the same checks.
- To prevent errors from incorrect argument order, especially for functions with multiple arguments of the same type (like coordinates), prefer passing arguments as a single object and using destructuring.
| export const ENV = { | ||
| API_BASE_URL: env.VITE_API_BASE_URL || `invalid`, // 에러나게 아무 뻥값 넣었습니다. | ||
| WS_BASE_URL: env.VITE_WS_BASE_URL || `invalid`, | ||
| } as const; |
There was a problem hiding this comment.
환경 변수가 설정되지 않았을 때 'invalid' 문자열을 기본값으로 사용하는 것은 나중에 예측하기 어려운 런타임 오류를 발생시킬 수 있습니다. 애플리케이션 시작 시점에 필요한 환경 변수가 없는 경우, 명시적으로 오류를 발생시켜 빠르게 문제를 인지하고 수정할 수 있도록 하는 것이 좋습니다.
| export const ENV = { | |
| API_BASE_URL: env.VITE_API_BASE_URL || `invalid`, // 에러나게 아무 뻥값 넣었습니다. | |
| WS_BASE_URL: env.VITE_WS_BASE_URL || `invalid`, | |
| } as const; | |
| function getEnv(key: keyof AppEnv): string { | |
| const value = env[key]; | |
| if (value === undefined || value === '') { | |
| throw new Error(`Missing environment variable: ${key}`); | |
| } | |
| return value; | |
| } | |
| export const ENV = { | |
| API_BASE_URL: getEnv('VITE_API_BASE_URL'), | |
| WS_BASE_URL: getEnv('VITE_WS_BASE_URL'), | |
| } as const; |
| } catch (e) { | ||
| this.handleError(e); | ||
| } | ||
| }, "user-action"); |
There was a problem hiding this comment.
트랜잭션 origin으로 문자열 리터럴 'user-action'을 직접 사용하고 있습니다. 유지보수성을 높이고 오타를 방지하기 위해, 이미 정의된 TRANSACTION_ORIGINS.USER_ACTION 상수를 사용하는 것이 좋습니다. 이 문제는 moveTo 메서드에서도 동일하게 발견됩니다.
| }, "user-action"); | |
| }, TRANSACTION_ORIGINS.USER_ACTION); |
| ? parentRealX + parentNode.width + this.config.xGap | ||
| : parentRealX - childNode.width - this.config.xGap; | ||
|
|
||
| console.log(realX, childNode); |
There was a problem hiding this comment.
디버깅 목적으로 추가된 console.log 문이 코드에 남아있습니다. 병합하기 전에 제거해야 합니다.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b31191ae29
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public deleteNode(nodeId: NodeId) { | ||
| this.container.getDoc().transact(() => { | ||
| this.container.delete({ nodeId }); | ||
| }); |
There was a problem hiding this comment.
deleteNode removes the subtree but never invalidates cached subtree heights or calls refreshLayout, and handleTransaction also skips layout recomputation for local transactions, so deletions leave surviving siblings at stale coordinates until some unrelated resize/content update happens. In practice, deleting a branch can leave large gaps or overlapping layout in the shared mindmap for all collaborators because no deletion path triggers a fresh layout pass.
Useful? React with 👍 / 👎.
2 participants
Closes #316
(작업중..)
목적
작업 내용
결과
(optional) 변경 전/후
(optional) 사용방법
(optional) 리뷰해줬으면 좋겠는 부분
(optional) 기타