feat(super-editor): add ui.comments.setActive for activate-only highlight (SD-3319)#3575
feat(super-editor): add ui.comments.setActive for activate-only highlight (SD-3319)#3575caio-pizzol wants to merge 1 commit into
Conversation
…nt highlight (SD-3319) Adds an activate-only method to the Custom UI comments handle so a consumer that scrolls a comment into view itself (e.g. ui.viewport rect plus its own scroll) can mark it active without using scrollTo. Routes through the existing setActiveComment command: highlight-only, no scroll, no selection move, and getSnapshot().activeIds stays selection-derived. Unknown ids (by id or importedId) are rejected so activating a missing id can't fade every other comment.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
No issues found across 3 files
Linked issue analysis
Linked issue: SD-3319: Add an activate-only comment API: ui.comments.setActive(commentId | null)
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Expose ui.comments.setActive(commentId | null) on the comments handle | The public handle and types were added so consumers can call setActive with a string id or null. |
| ✅ | Activate a comment highlight without scrolling, moving selection, or focusing the editor | Implementation routes to the presentation editor's setActiveComment and intentionally avoids navigation/selection commands; tests assert no navigateTo or setCursorById calls. |
| ✅ | Accept null to clear the active highlight | setActive(null) is implemented and a test verifies it clears (calls setActiveComment with commentId: null). |
| ✅ | Validate non-null ids against current comments, matching native id or imported Word id | Before dispatching, the code checks the editor's comment list for a matching id or importedId; a test verifies importedId is accepted. |
| ✅ | Return false for unknown ids and avoid dispatching (so unknown id doesn't fade every comment) | The code returns false when a non-null id is not found and the tests assert the underlying command is not called and false is returned. |
| ✅ | Return a boolean and pass through a false result from the underlying command | setActive returns the boolean result of setActiveComment === true; tests ensure a false from the command is propagated. |
| ✅ | Do not change getSnapshot().activeIds (keep document highlight decoupled from snapshot activeIds) | Implementation doesn't call refresh/notify and tests assert the snapshot activeIds remain unchanged after calling setActive. |
| ✅ | Return false when no editor is mounted | The code resolves the host editor and returns false when setActiveComment isn't available; there's a test for the no-editor case. |
Tip: cubic could auto-approve low-risk PRs like this, if it thinks it's safe to merge. Learn more
Re-trigger cubic
![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: 139765cf1f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // present in the current comment list — matching by `id` or the | ||
| // imported Word id, both of which the painter keys on. | ||
| const items = editor?.doc?.comments?.list?.().items ?? []; | ||
| const known = items.some((c) => c.id === commentId || c.importedId === commentId); |
There was a problem hiding this comment.
When commentId is a reply id from ui.comments.reply, comments.list() includes that row, so this guard accepts it, but replies are created only as metadata (parentCommentId) and do not add a comment mark to the document. The highlight decorator only matches painted data-comment-ids/imported aliases, so activating a reply id matches no DOM element and falls into the same “fade every other comment” state this guard is trying to prevent; this affects consumers that call setActive from a comments sidebar on a reply rather than the root thread. Consider rejecting items with a parentCommentId here or mapping them to the anchored parent id before dispatching.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
Gives the Custom UI comments handle a way to highlight a comment in the document without scrolling to it. A consumer that brings a comment into view on its own (a
ui.viewportrect plus its own scroll) had no public way to mark it active afterward; the only path,scrollTo, also moves the caret and scrolls.setActive(commentId)highlights a comment,setActive(null)clears it. It routes through the existing internal activation, so it does not scroll, move the selection, or focus the editor.getSnapshot().activeIdsstays selection-derived and is left untouched, so the document highlight and the snapshot's active set are intentionally separate. An id that matches no current comment returnsfalserather than dimming every other comment.Review: is the
activeIdsdivergence acceptable for v1 (documented on the method), and isbooleanthe return shape we want vs a receipt? Those are the two open API calls.Verified:
vitest src/ui/comments.test.ts→ 29 passed;pnpm check:public:superdoc→ 13/13 stages PASS (rebased on current main)