feat(TKPlugin): implemented list-item-level TK detection and positioning #1606
Conversation
…ning Previously, TK monikers in bulleted lists were tracked at the <ul> level, making it difficult for authors to identify which specific list items contained incomplete content. This change implements list-aware TK detection that treats each <li> element as an individual container. - Added getEffectiveTopLevelElement() to recognize list items as containers - Updated mutation listener to use list-aware detection logic - Enhanced positioning calculations for precise list item indicator placement - Added comprehensive test coverage for list-specific TK behavior This provides authors with granular visibility into which list items need completion, consistent with how TK tracking works for paragraph elements. Fixes PROD-2389
lsinger
requested review from
PaulAdamDavis,
kevinansfield and
minimaluminium
|
Warning Rate limit exceeded@lsinger has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. 📒 Files selected for processing (1)
WalkthroughTreats list items (LI) as the effective top-level container for TK nodes by adding getEffectiveTopLevelElement(node). Introduces indicator positioning constants and updates TKIndicator.calculatePosition to prefer an LI or a card element as the positioning target, with early returns when no container exists. Per-key element lookups are cached and missing elements are skipped. TKPlugin mutation handling derives parentNodeKey from the effective top-level element. TKIndicators rendering returns null for missing parentContainer. Adds two e2e tests verifying per-list-item indicator creation and distinct vertical positioning. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. 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: 1
🧹 Nitpick comments (4)
packages/koenig-lexical/test/e2e/plugins/TKPlugin.test.js (2)
118-131: Good coverage; add a quick guard that we indeed created 4 list itemsThis avoids false positives if list auto‑continuation ever changes.
Apply this diff:
await page.keyboard.type('Fourth item with TK'); - // Should have 3 separate TK indicators, one for each list item containing TK + // Sanity check: we created a 4-item list + await expect(page.getByRole('listitem')).toHaveCount(4); + + // Should have 3 separate TK indicators, one for each list item containing TK await expect(page.getByTestId('tk-indicator')).toHaveCount(3);
133-152: Stabilize position assertions to avoid flake from null bounding boxesEnsure indicators are visible and add a small guard before reading positions.
Apply this diff:
const indicators = page.getByTestId('tk-indicator'); await expect(indicators).toHaveCount(2); + // Ensure they are visible and we're indeed in a 2-item list + await expect(page.getByRole('listitem')).toHaveCount(2); + // Get positions to verify they're different (one per list item) const firstIndicator = indicators.first(); const secondIndicator = indicators.last(); - const firstBox = await firstIndicator.boundingBox(); - const secondBox = await secondIndicator.boundingBox(); + await expect(firstIndicator).toBeVisible(); + await expect(secondIndicator).toBeVisible(); + const firstBox = await firstIndicator.boundingBox(); + const secondBox = await secondIndicator.boundingBox(); + expect(firstBox).not.toBeNull(); + expect(secondBox).not.toBeNull(); // The indicators should have different vertical positions expect(Math.abs(firstBox.y - secondBox.y)).toBeGreaterThan(10);packages/koenig-lexical/src/plugins/TKPlugin.jsx (2)
53-59: Nice LI anchoring; also observe the card element to reduce reposition lagWhen anchoring to a card within a non‑LI container, observe that card so indicator updates on card resizes.
Apply this diff in the resize observer effect:
useEffect(() => { const observer = new ResizeObserver(() => (setPosition(calculatePosition()))); observer.observe(rootElement); observer.observe(containingElement); + // If positioned against a nested card, observe it too + const cardEl = containingElement.nodeName !== 'LI' + ? containingElement.querySelector('[data-kg-card]') + : null; + if (cardEl) { + observer.observe(cardEl); + } return () => { observer.disconnect(); }; }, [rootElement, containingElement, calculatePosition]);
192-196: Guard against missing parent key to avoid “undefined” bucketsIf effectiveTopLevel is null or DOM not yet attached, parentNodeKey can be undefined; skip until next mutation to avoid polluting tkNodeMap.
Apply this diff:
-const effectiveTopLevel = getEffectiveTopLevelElement($getNodeByKey(tkNodeKey)); -const parentNodeKey = effectiveTopLevel?.getKey(); -const topLevelNodeKey = parentEditorNodeKey || parentNodeKey; -addEditorTkNode(editor.getKey(), topLevelNodeKey, tkNodeKey); +const effectiveTopLevel = getEffectiveTopLevelElement($getNodeByKey(tkNodeKey)); +const parentNodeKey = effectiveTopLevel?.getKey(); +if (!parentEditorNodeKey && !parentNodeKey) { + // Node not yet attached to a top-level container; try on next mutation + continue; +} +const topLevelNodeKey = parentEditorNodeKey || parentNodeKey; +addEditorTkNode(editor.getKey(), topLevelNodeKey, tkNodeKey);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/koenig-lexical/src/plugins/TKPlugin.jsx(3 hunks)packages/koenig-lexical/test/e2e/plugins/TKPlugin.test.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/koenig-lexical/test/e2e/plugins/TKPlugin.test.js (1)
packages/koenig-lexical/test/utils/e2e.js (1)
focusEditor(58-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
…ction Fixed TypeError in getEffectiveTopLevelElement where currentNode.isRoot() method doesn't exist. Replaced with proper Lexical node parent checking using currentNode.getParent() !== null to traverse the node hierarchy correctly when identifying list items as TK containers. Fixes PROD-2389
- Add INDICATOR_OFFSET_RIGHT and INDICATOR_OFFSET_TOP constants - Add NODE_TYPES constant for element type checking - Improves code readability and maintainability
- Use early return for non-list elements for better readability - Use optional chaining for safer null checking in while loop - Add explicit fallback comment for clarity - Restructure logic flow to be more maintainable
- Replace conditional assignment with ternary operator - Use NODE_TYPES constant for element type checking - Eliminate redundant element queries - More concise and readable code
- Add null check for containingElement in TKIndicator - Add element validation in toggleHighlightClasses forEach loop - Return null instead of false for consistency in map filter - Prevents potential runtime errors when DOM elements are unavailable
- Move null check for containingElement after all hooks - Add proper curly braces for if statement return - Ensures hooks are called in consistent order per React rules
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/koenig-lexical/src/plugins/TKPlugin.jsx (1)
4-4: Good call switching to Lexical list helpersUsing $isListItemNode/$isListNode avoids brittle DOM/nodeName checks and aligns with Lexical’s model.
🧹 Nitpick comments (3)
packages/koenig-lexical/src/plugins/TKPlugin.jsx (3)
15-18: Hard-coded offsets: consider theming or CSS varsIf these need tweaking across themes, prefer deriving from editor theme or CSS variables to avoid magic numbers in JS.
19-23: Tiny: drop NODE_TYPES indirectionComparing containingElement.tagName === 'LI' is sufficiently clear; the extra constant adds indirection without payoff.
Apply this small refactor:
-// Node type constants -const NODE_TYPES = { - LIST_ITEM: 'LI' -};And where used:
-const positioningElement = containingElement.nodeName === NODE_TYPES.LIST_ITEM +const positioningElement = containingElement.tagName === 'LI' ? containingElement : containingElement.querySelector('[data-kg-card]') || containingElement;
24-49: List-aware top-level resolution looks rightLogic is correct and guarded for null topLevel. Minor robustness: stop at root explicitly in the climb.
Apply:
-while (currentNode?.getParent()) { +while (currentNode && !currentNode.isRoot()) { if ($isListItemNode(currentNode)) { return currentNode; } currentNode = currentNode.getParent(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/koenig-lexical/src/plugins/TKPlugin.jsx(5 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
packages/koenig-lexical/src/plugins/TKPlugin.jsx
[error] 62-62: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Node 20.11.1
- GitHub Check: Node 22.13.1
🔇 Additional comments (3)
packages/koenig-lexical/src/plugins/TKPlugin.jsx (3)
68-72: Positioning target selection is sensiblePreferring LI as anchor and otherwise falling back to [data-kg-card] or the container should give stable placement in lists and cards.
75-76: Offset math reads wellUsing INDICATOR_OFFSET_TOP atop the container rect delta keeps vertical alignment predictable.
295-312: Null filtering on missing parent containers is fineReturning null and filtering via .filter(Boolean) keeps the render list clean.
| // Early return if containing element is not available | ||
| if (!containingElement) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
React hooks called after an early return (violates Hooks rules)
TKIndicator returns null before declaring hooks, then declares useCallback/useState/useEffect in other renders. This breaks hook ordering and can crash at runtime.
Apply this fix: remove the early return and guard inside hooks; only return null just before rendering.
-// Early return if containing element is not available
-if (!containingElement) {
- return null;
-}
+// No early return here; guard inside hooks and at render time
// position element relative to the TK Node containing element
const calculatePosition = useCallback(() => {
- let top = 0;
- let right = INDICATOR_OFFSET_RIGHT;
+ // Default position when container is missing
+ if (!containingElement) {
+ return {top: 0, right: INDICATOR_OFFSET_RIGHT};
+ }
+ let top = 0;
+ let right = INDICATOR_OFFSET_RIGHT;
const rootElementRect = rootElement.getBoundingClientRect();
// Determine positioning element based on container type
- const positioningElement = containingElement.nodeName === NODE_TYPES.LIST_ITEM
+ const positioningElement = containingElement.tagName === 'LI'
? containingElement
: containingElement.querySelector('[data-kg-card]') || containingElement;
const positioningElementRect = positioningElement.getBoundingClientRect();
top = positioningElementRect.top - rootElementRect.top + INDICATOR_OFFSET_TOP;
if (positioningElementRect.right > rootElementRect.right) {
right = right - (positioningElementRect.right - rootElementRect.right);
}
return {top, right};
}, [rootElement, containingElement]);
const [position, setPosition] = useState(calculatePosition());
// set up an observer to reposition the indicator when the TK node containing
// element moves relative to the root element
useEffect(() => {
const observer = new ResizeObserver(() => (setPosition(calculatePosition())));
observer.observe(rootElement);
- observer.observe(containingElement);
+ if (containingElement) {
+ observer.observe(containingElement);
+ }
return () => {
observer.disconnect();
};
}, [rootElement, containingElement, calculatePosition]);
const style = {
top: `${position.top}px`,
right: `${position.right}px`
};
return (
+ // Render nothing when container isn't available
+ !containingElement ? null :
<div
className="absolute cursor-pointer p-1 text-2xs font-medium text-grey-600"
data-testid="tk-indicator"
style={style}
onClick={onClick}
onMouseEnter={onMouseEnter}
onMouseLeave={onMouseLeave}
>TK</div>
);Also applies to: 62-86, 153-165, 171-181
🤖 Prompt for AI Agents
In packages/koenig-lexical/src/plugins/TKPlugin.jsx around lines 56-60 (and
similarly for ranges 62-86, 153-165, 171-181), the component returns null before
any hook declarations which breaks the Rules of Hooks; remove the early return
so hooks (useState/useCallback/useEffect) are always declared in the same order,
add conditional guards inside the hook bodies or their callbacks to no-op when
containingElement is null/undefined, and only perform a final conditional return
null immediately before the JSX render so rendering still short-circuits when
the containing element is missing.
| const element = editor.getElementByKey(key); | ||
| if (!element) return; | ||
|
|
||
| if (isHighlighted) { | ||
| editor.getElementByKey(key).classList.remove(...tkClasses); | ||
| editor.getElementByKey(key).classList.add(...tkHighlightClasses); | ||
| element.classList.remove(...tkClasses); | ||
| element.classList.add(...tkHighlightClasses); | ||
| } else { | ||
| editor.getElementByKey(key).classList.add(...tkClasses); | ||
| editor.getElementByKey(key).classList.remove(...tkHighlightClasses); | ||
| element.classList.add(...tkClasses); | ||
| element.classList.remove(...tkHighlightClasses); | ||
| } |
There was a problem hiding this comment.
Potential TypeError: classList.add/remove called with 0 args or empty tokens
If theme.tk or theme.tkHighlighted is undefined or contains extra spaces, tkClasses/tkHighlightClasses can be [], leading to DOMTokenList.add()/remove() with no args (TypeError) or empty token '' (also TypeError).
Apply guards and sanitize tokens:
- nodeKeys.forEach((key) => {
- const element = editor.getElementByKey(key);
- if (!element) return;
-
- if (isHighlighted) {
- element.classList.remove(...tkClasses);
- element.classList.add(...tkHighlightClasses);
- } else {
- element.classList.add(...tkClasses);
- element.classList.remove(...tkHighlightClasses);
- }
- });
+ const baseClasses = tkClasses.filter(Boolean);
+ const highlightClasses = tkHighlightClasses.filter(Boolean);
+
+ nodeKeys.forEach((key) => {
+ const element = editor.getElementByKey(key);
+ if (!element) return;
+
+ if (isHighlighted) {
+ if (baseClasses.length) element.classList.remove(...baseClasses);
+ if (highlightClasses.length) element.classList.add(...highlightClasses);
+ } else {
+ if (baseClasses.length) element.classList.add(...baseClasses);
+ if (highlightClasses.length) element.classList.remove(...highlightClasses);
+ }
+ });Optional: also sanitize at source:
-const tkClasses = editor._config.theme.tk?.split(' ') || [];
-const tkHighlightClasses = editor._config.theme.tkHighlighted?.split(' ') || [];
+const tkClasses = (editor._config.theme.tk || '').split(/\s+/).filter(Boolean);
+const tkHighlightClasses = (editor._config.theme.tkHighlighted || '').split(/\s+/).filter(Boolean);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const element = editor.getElementByKey(key); | |
| if (!element) return; | |
| if (isHighlighted) { | |
| editor.getElementByKey(key).classList.remove(...tkClasses); | |
| editor.getElementByKey(key).classList.add(...tkHighlightClasses); | |
| element.classList.remove(...tkClasses); | |
| element.classList.add(...tkHighlightClasses); | |
| } else { | |
| editor.getElementByKey(key).classList.add(...tkClasses); | |
| editor.getElementByKey(key).classList.remove(...tkHighlightClasses); | |
| element.classList.add(...tkClasses); | |
| element.classList.remove(...tkHighlightClasses); | |
| } | |
| const baseClasses = tkClasses.filter(Boolean); | |
| const highlightClasses = tkHighlightClasses.filter(Boolean); | |
| nodeKeys.forEach((key) => { | |
| const element = editor.getElementByKey(key); | |
| if (!element) return; | |
| if (isHighlighted) { | |
| if (baseClasses.length) element.classList.remove(...baseClasses); | |
| if (highlightClasses.length) element.classList.add(...highlightClasses); | |
| } else { | |
| if (baseClasses.length) element.classList.add(...baseClasses); | |
| if (highlightClasses.length) element.classList.remove(...highlightClasses); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In packages/koenig-lexical/src/plugins/TKPlugin.jsx around lines 131 to 140,
tkClasses and tkHighlightClasses may be empty or contain empty tokens which
causes classList.add/remove to throw; sanitize the token arrays (trim strings,
filter out empty strings) when building tkClasses/tkHighlightClasses and only
call element.classList.add(...) or .remove(...) if the resulting array length >
0 (i.e. guard the calls), and optionally ensure the theme.tk and
theme.tkHighlighted source values are normalized (split on whitespace, trim,
filter) to prevent empty tokens upstream.
| const effectiveTopLevel = getEffectiveTopLevelElement($getNodeByKey(tkNodeKey)); | ||
| const parentNodeKey = effectiveTopLevel?.getKey(); | ||
| const topLevelNodeKey = parentEditorNodeKey || parentNodeKey; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Guard addEditorTkNode when no container key is resolved
Effective container can be null in rare cases; avoid inserting undefined keys into tkNodeMap.
Apply:
-const topLevelNodeKey = parentEditorNodeKey || parentNodeKey;
-addEditorTkNode(editor.getKey(), topLevelNodeKey, tkNodeKey);
+const topLevelNodeKey = parentEditorNodeKey || parentNodeKey;
+if (topLevelNodeKey) {
+ addEditorTkNode(editor.getKey(), topLevelNodeKey, tkNodeKey);
+}If you suspect this never happens in practice, quick check:
🏁 Script executed:
#!/bin/bash
# Look for any safeguards already in addEditorTkNode implementation
rg -nC2 "function\s+addEditorTkNode|addEditorTkNode\s*\(" --type=js --type=jsxLength of output: 27
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "Git branch: $(git rev-parse --abbrev-ref HEAD || true)"
echo "--- Search for addEditorTkNode occurrences ---"
rg -nC3 "addEditorTkNode" || true
echo "--- Search for possible definitions ---"
rg -nC3 "function\s+addEditorTkNode" || true
rg -nC3 "const\s+addEditorTkNode\s*=" || true
rg -nC3 "export\s+function\s+addEditorTkNode" || true
echo "--- Search for tkNodeMap occurrences ---"
rg -nC3 "tkNodeMap" || true
echo "--- Show packages/koenig-lexical/src/plugins/TKPlugin.jsx (first 300 lines) ---"
if [ -f "packages/koenig-lexical/src/plugins/TKPlugin.jsx" ]; then
sed -n '1,300p' packages/koenig-lexical/src/plugins/TKPlugin.jsx
else
echo "TKPlugin.jsx not found at expected path; listing directory:"
ls -la packages/koenig-lexical/src/plugins || true
fiLength of output: 18029
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "---- show addEditorTkNode occurrences ----"
rg -nC3 "addEditorTkNode" packages/koenig-lexical || true
echo "---- print TKContext.jsx ----"
if [ -f "packages/koenig-lexical/src/context/TKContext.jsx" ]; then
sed -n '1,260p' packages/koenig-lexical/src/context/TKContext.jsx
echo "---- remainder ----"
sed -n '261,520p' packages/koenig-lexical/src/context/TKContext.jsx || true
else
echo "packages/koenig-lexical/src/context/TKContext.jsx not found"
ls -la packages/koenig-lexical/src || true
fiLength of output: 6996
Guard addEditorTkNode when no container key is resolved
Effective container can be null; avoid inserting undefined keys into tkNodeMap.
Apply:
-const topLevelNodeKey = parentEditorNodeKey || parentNodeKey;
-addEditorTkNode(editor.getKey(), topLevelNodeKey, tkNodeKey);
+const topLevelNodeKey = parentEditorNodeKey || parentNodeKey;
+if (topLevelNodeKey) {
+ addEditorTkNode(editor.getKey(), topLevelNodeKey, tkNodeKey);
+}File: packages/koenig-lexical/src/plugins/TKPlugin.jsx (around lines 208-211)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const effectiveTopLevel = getEffectiveTopLevelElement($getNodeByKey(tkNodeKey)); | |
| const parentNodeKey = effectiveTopLevel?.getKey(); | |
| const topLevelNodeKey = parentEditorNodeKey || parentNodeKey; | |
| const effectiveTopLevel = getEffectiveTopLevelElement($getNodeByKey(tkNodeKey)); | |
| const parentNodeKey = effectiveTopLevel?.getKey(); | |
| const topLevelNodeKey = parentEditorNodeKey || parentNodeKey; | |
| if (topLevelNodeKey) { | |
| addEditorTkNode(editor.getKey(), topLevelNodeKey, tkNodeKey); | |
| } |
🤖 Prompt for AI Agents
In packages/koenig-lexical/src/plugins/TKPlugin.jsx around lines 208-210, the
code computes topLevelNodeKey from an effective top-level element which can be
null and may lead to inserting undefined into tkNodeMap; before calling
addEditorTkNode (or otherwise adding entries keyed by topLevelNodeKey), guard by
checking that topLevelNodeKey is truthy (e.g., if (!topLevelNodeKey)
return/continue or skip the add) so no undefined keys are inserted into
tkNodeMap.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/koenig-lexical/src/plugins/TKPlugin.jsx (2)
50-53: Sanitize class tokens; guard classList.add/remove to avoid TypeErrorEmpty/undefined theme tokens lead to classList.add/remove being called with 0 args.
Apply:
- const tkClasses = editor._config.theme.tk?.split(' ') || []; - const tkHighlightClasses = editor._config.theme.tkHighlighted?.split(' ') || []; + const tkClasses = (editor._config.theme.tk || '').split(/\s+/).filter(Boolean); + const tkHighlightClasses = (editor._config.theme.tkHighlighted || '').split(/\s+/).filter(Boolean); @@ - if (isHighlighted) { - element.classList.remove(...tkClasses); - element.classList.add(...tkHighlightClasses); - } else { - element.classList.add(...tkClasses); - element.classList.remove(...tkHighlightClasses); - } + if (isHighlighted) { + if (tkClasses.length) element.classList.remove(...tkClasses); + if (tkHighlightClasses.length) element.classList.add(...tkHighlightClasses); + } else { + if (tkClasses.length) element.classList.add(...tkClasses); + if (tkHighlightClasses.length) element.classList.remove(...tkHighlightClasses); + }Also applies to: 131-137
210-214: Guard addEditorTkNode when no container key is resolvedAvoid inserting undefined keys into tkNodeMap.
- const topLevelNodeKey = parentEditorNodeKey || parentNodeKey; - addEditorTkNode(editor.getKey(), topLevelNodeKey, tkNodeKey); + const topLevelNodeKey = parentEditorNodeKey || parentNodeKey; + if (topLevelNodeKey) { + addEditorTkNode(editor.getKey(), topLevelNodeKey, tkNodeKey); + }
🧹 Nitpick comments (2)
packages/koenig-lexical/src/plugins/TKPlugin.jsx (2)
15-18: Optional: make horizontal positioning RTL-awareRight-only offset misplaces indicators in RTL. Compute side from root direction.
-const INDICATOR_OFFSET_RIGHT = -56; +const INDICATOR_OFFSET_RIGHT = -56; // default inline-end offset for LTR @@ - const style = { - top: `${position.top}px`, - right: `${position.right}px` - }; + const direction = getComputedStyle(rootElement).direction; + const style = direction === 'rtl' + ? {top: `${position.top}px`, left: `${position.right}px`} + : {top: `${position.top}px`, right: `${position.right}px`};Also applies to: 168-171
19-23: Nit: drop NODE_TYPES indirection; use tagName directlySimplify by removing NODE_TYPES and checking tagName.
-// Node type constants -const NODE_TYPES = { - LIST_ITEM: 'LI' -}; @@ - const positioningElement = containingElement.nodeName === NODE_TYPES.LIST_ITEM + const positioningElement = containingElement.tagName === 'LI' ? containingElement : containingElement.querySelector('[data-kg-card]') || containingElement;Also applies to: 63-67
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/koenig-lexical/src/plugins/TKPlugin.jsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/koenig-lexical/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
packages/koenig-lexical/**/*.{tsx,jsx}: Use Tailwind CSS utility classes where possible in React components
Import SVGs as React components instead of using them as image sources
Files:
packages/koenig-lexical/src/plugins/TKPlugin.jsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (4)
packages/koenig-lexical/src/plugins/TKPlugin.jsx (4)
4-4: Good: list-aware imports addedUsing $isListNode/$isListItemNode is the right approach for robust list detection.
24-49: Effective container resolution: LGTM (please verify nested lists)Logic correctly prefers the nearest LI when top-level is a list. Consider adding a test for nested lists (LI > UL/OL > LI) to ensure the nearest LI is used.
163-167: Hooks order fix: LGTMEarly return moved after hooks; complies with Rules of Hooks.
297-303: TKIndicators: null-guard render is correctReturning null (then filtered) avoids rendering indicators for missing containers.
| // position element relative to the TK Node containing element | ||
| const calculatePosition = useCallback(() => { | ||
| let top = 0; | ||
| let right = -56; | ||
| let right = INDICATOR_OFFSET_RIGHT; | ||
|
|
||
| const rootElementRect = rootElement.getBoundingClientRect(); | ||
|
|
||
| const positioningElement = containingElement.querySelector('[data-kg-card]') || containingElement; | ||
| // Determine positioning element based on container type | ||
| const positioningElement = containingElement.nodeName === NODE_TYPES.LIST_ITEM | ||
| ? containingElement | ||
| : containingElement.querySelector('[data-kg-card]') || containingElement; | ||
|
|
There was a problem hiding this comment.
Fix crash when containingElement is null (initial render + observer)
calculatePosition dereferences containingElement before the post-hook early return; useEffect also observes a null element. Both can throw.
Apply:
- const calculatePosition = useCallback(() => {
- let top = 0;
- let right = INDICATOR_OFFSET_RIGHT;
+ const calculatePosition = useCallback(() => {
+ // Guard: container may be missing during initial renders/mutations
+ if (!containingElement) {
+ return {top: 0, right: INDICATOR_OFFSET_RIGHT};
+ }
+ let top = 0;
+ let right = INDICATOR_OFFSET_RIGHT;
@@
- const positioningElement = containingElement.nodeName === NODE_TYPES.LIST_ITEM
+ const positioningElement = (containingElement.tagName === 'LI')
? containingElement
: containingElement.querySelector('[data-kg-card]') || containingElement;
@@
- const [position, setPosition] = useState(calculatePosition());
+ const [position, setPosition] = useState(calculatePosition());
@@
- observer.observe(rootElement);
- observer.observe(containingElement);
+ observer.observe(rootElement);
+ if (containingElement) {
+ observer.observe(containingElement);
+ }Also applies to: 79-79, 150-161
🤖 Prompt for AI Agents
In packages/koenig-lexical/src/plugins/TKPlugin.jsx around lines 56-67 (and
additionally at 79 and 150-161), calculatePosition and the useEffect observer
assume containingElement (and rootElement) are non-null and call methods on them
causing crashes on initial render or when the observer supplies null; add
null/undefined guards at the start of calculatePosition and in the useEffect
callbacks so they return early if containingElement or rootElement is falsy, and
when determining positioningElement only access containingElement.nodeName or
querySelector after confirming containingElement exists; ensure the callback
dependencies remain correct and that any observers only attach when the element
references are present.
- Extract positioning logic into dedicated helper functions - Consolidate getPositioningElement and calculateRightOffset functions - Simplify calculatePosition callback with cleaner, more readable flow - Improve separation of concerns and maintainability
1 participant
Previously, TK monikers in bulleted lists were tracked at the
level,- element as an individual container.
- Added getEffectiveTopLevelElement() to recognize list items as containers
- Updated mutation listener to use list-aware detection logic
- Enhanced positioning calculations for precise list item indicator placement
- Added comprehensive test coverage for list-specific TK behavior
making it difficult for authors to identify which specific list items
contained incomplete content. This change implements list-aware TK detection
that treats each
This provides authors with granular visibility into which list items need
completion, consistent with how TK tracking works for paragraph elements.
Fixes PROD-2389