feat(comments): add scrollToComment API#1777
feat(comments): add scrollToComment API#1777financialvice wants to merge 1 commit intosuperdoc-dev:mainfrom
Conversation
![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: 67a14fef99
ℹ️ 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".
| if (!commentsConfig || commentsConfig === false) return false; | ||
| if (!commentId || typeof commentId !== 'string') return false; | ||
|
|
||
| const element = document.querySelector(`[data-thread-id="${commentId}"]`); |
There was a problem hiding this comment.
Because document.querySelector searches the whole page, this new API will scroll to the first matching data-thread-id across all SuperDoc instances. If two editors load documents with overlapping thread ids (e.g., separate docs both have comment-1), calling scrollToComment on one instance can scroll the other instance instead. You already expose this.element as the container; scoping the lookup to that element avoids cross-instance collisions.
Useful? React with 👍 / 👎.
github-actions
Bot
added
review: quick
and removed
review: thorough
review: quick
labels
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@financialvice the overall approach is solid — good input validation, boolean return, and optional chaining are all improvements over the internal Vue equivalent. left a few inline comments. one of them (the CSS.escape one) is worth fixing before merging since it affects the error contract of the public API; the other is just a suggestion.
| if (!commentId || typeof commentId !== 'string') return false; | ||
|
|
||
| const element = document.querySelector(`[data-thread-id="${commentId}"]`); | ||
| if (!element) return false; |
There was a problem hiding this comment.
if commentId contains characters like ", \, or ], this will throw a DOMException instead of returning false. the codebase already uses CSS.escape for this (e.g. SuperEditor.vue:803), so worth applying here too.
| if (!element) return false; | |
| const element = document.querySelector(`[data-thread-id="${CSS.escape(commentId)}"]`); |
| }); | ||
|
|
||
| it('scrolls to a comment and sets it active', async () => { | ||
| const { commentsStore } = createAppHarness(); |
There was a problem hiding this comment.
the tests only cover the happy path. a case where the element isn't in the DOM (expecting false) would catch the most likely real-world failure mode — worth adding?
|
@financialvice thanks for this! We're moving your work to a new PR so we can wrap up the remaining changes and get it merged. Closing in favor of #2440 |
2 participants
Exposes a public scrollToComment method for custom comment UIs.\n\nTests: pnpm -C packages/superdoc test -- SuperDoc.test.js