fix: text tooling unblock for ruby tags#61
Open
KirylHatalski wants to merge 2 commits intodevelopfrom
Open
Conversation
KirylHatalski
requested review from
a team,
Karol-Stelmaczonek,
bartlomiejmarszal,
pnal and
tikhanovichA
and removed request for
a team
WalkthroughPlugin adjusts toolbar command state logic for furigana (ruby) selections: it collects a subset of stateless toolbar buttons, detects when the selection is inside an Changes
Sequence Diagram(s)sequenceDiagram
participant Editor
participant TaofuriganaPlugin
participant Toolbar
participant Buttons
Editor->>TaofuriganaPlugin: selection change / refreshCommandState()
TaofuriganaPlugin->>TaofuriganaPlugin: collect statelessButtons from toolbar
TaofuriganaPlugin->>TaofuriganaPlugin: isInRuby? / isInRtFugirana?
alt in ruby and deleteRubyIfNoRt() == false
TaofuriganaPlugin->>Toolbar: set rubyFurigana = TRISTATE_ON
Note right of TaofuriganaPlugin: start 150ms timeout
par after 150ms
TaofuriganaPlugin->>Toolbar: setButtonsState(TRISTATE_DISABLED)
TaofuriganaPlugin->>Buttons: set each statelessButton -> TRISTATE_OFF
end
else in rt (or other cases)
TaofuriganaPlugin->>Toolbar: set rubyFurigana = TRISTATE_ON
Note right of TaofuriganaPlugin: existing delayed setButtonsState(TRISTATE_DISABLED)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/taofurigana/plugin.js`:
- Around line 96-101: The isInRtFugirana function incorrectly omits the
includeSelf flag when checking ancestors, so when the current node is the <rt>
element it returns false; update the call to node.getAscendant('rt') inside
isInRtFugirana to pass includeSelf: true (i.e., getAscendant('rt', {
includeSelf: true })) so the current <rt> node is counted as an ancestor and the
function correctly returns true for nodes that are the <rt> itself.
- Around line 223-229: The toolbar buttons are visually re-enabled but their
commands remain disabled because the code calls button.setState(...) instead of
updating the underlying command state; change the loop over statelessButtons
(where currently button.setState(CKEDITOR.TRISTATE_OFF)) to fetch each command
via editor.getCommand(button) and call .setState(CKEDITOR.TRISTATE_OFF) so both
UI and command states are reset; ensure this change is applied in the block
following setButtonsState(CKEDITOR.TRISTATE_DISABLED) and reference
setButtonsState, statelessButtons, button.setState, and
editor.getCommand(...).setState when locating the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 87f9a984-57d8-426f-b1f7-afd3588c82ee
📒 Files selected for processing (1)
plugins/taofurigana/plugin.js
bartlomiejmarszal
approved these changes
Apr 15, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/taofurigana/plugin.js (1)
223-230: Track timeout IDs to prevent accumulation from rapid selection changes.The mouseup and keyup listeners on lines 332-335 repeatedly call
refreshCommandState(), which schedules two separate setTimeout calls on lines 225-230 and 233-235. Without tracking timeout IDs, rapid events can accumulate unresolved timeouts, causing a delayed timeout to fire after a newer state change, resulting in incorrect button states.This pattern is used throughout the codebase (menu, notification, magicline, floatpanel, clipboard plugins). Add a plugin-level
buttonStateTimeoutvariable and clear previous timeouts before scheduling new ones.Proposed fix
Add variable declaration after line 11:
var statelessButtons = []; + var buttonStateTimeout;Update both timeout blocks:
} else if (!isInRtFugirana(range.startContainer)) { command.setState(CKEDITOR.TRISTATE_ON); + clearTimeout(buttonStateTimeout); - setTimeout(function () { + buttonStateTimeout = setTimeout(function () { setButtonsState(CKEDITOR.TRISTATE_DISABLED); statelessButtons.forEach(function (button) { button.setState(CKEDITOR.TRISTATE_OFF); }); }, 150); } else { command.setState(CKEDITOR.TRISTATE_ON); + clearTimeout(buttonStateTimeout); - setTimeout(function () { + buttonStateTimeout = setTimeout(function () { setButtonsState(CKEDITOR.TRISTATE_DISABLED); }, 150);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/taofurigana/plugin.js` around lines 223 - 230, Add a plugin-level timeout tracker (e.g., buttonStateTimeout) and clear any existing timeout before scheduling new ones in the blocks that call setTimeout from refreshCommandState; specifically, declare buttonStateTimeout at plugin scope and in the blocks where you call setTimeout to call clearTimeout(buttonStateTimeout) prior to assigning buttonStateTimeout = setTimeout(...). Update both timeout usages that setButtonsState(...) and update statelessButtons.forEach(...) so the previous delayed handler is cancelled and only the latest timeout can fire.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/taofurigana/plugin.js`:
- Around line 223-230: Add a plugin-level timeout tracker (e.g.,
buttonStateTimeout) and clear any existing timeout before scheduling new ones in
the blocks that call setTimeout from refreshCommandState; specifically, declare
buttonStateTimeout at plugin scope and in the blocks where you call setTimeout
to call clearTimeout(buttonStateTimeout) prior to assigning buttonStateTimeout =
setTimeout(...). Update both timeout usages that setButtonsState(...) and update
statelessButtons.forEach(...) so the previous delayed handler is cancelled and
only the latest timeout can fire.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c47ce7af-603f-49c8-a7b6-d59e8d6e248a
📒 Files selected for processing (1)
plugins/taofurigana/plugin.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the purpose of this pull request?
https://oat-sa.atlassian.net/browse/AUT-4506
added whitelist for state less buttons for ruby tag text content
Summary by CodeRabbit