Try to fix race condition in ParsedText

[nygrenh]

Summary by CodeRabbit

• Bug Fixes
  • Enhanced glossary target detection mechanism to ensure glossary terms are reliably identified and processed, reducing instances of missed or incorrectly handled terms in course materials.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The ParsedText component's glossary target scanning logic is refactored to use a multi-pass rescan mechanism with delayed iterations (0, 50, 200 ms) instead of a single-pass extraction, including utilities for target tracking, comparison, and validity verification, plus timer cleanup.

Changes

Cohort / File(s)	Summary
Glossary Target Rescan Mechanism services/course-material/src/components/ParsedText/index.tsx	Replaces single-pass target extraction with initial scan followed by conditional, timed rescans; adds utilities to track, compare, and validate targets; implements timer cleanup on unmount or dependency changes; logs warnings on rescan triggers; only updates state when target set differs

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Timer and async logic: Multiple delayed scans (0, 50, 200 ms) require verification of proper cleanup and edge-case handling
• State update conditionals: Logic for comparing target sets and selective state updates needs validation
• Dependency and unmount handling: Ensure timers are properly cleared and no stale closures remain
• Utility functions: Verify target tracking, comparison, and validity verification logic is correct

Suggested labels

bug

Poem

🐰 A glossary fair needs scanning twice,
With gentle delays, oh so nice!
Targets now tracked with utmost care,
Timers cleaned up with flair,
Parse your text without a scare! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly addresses the main objective of fixing a race condition in the ParsedText component, which aligns with the changeset that introduces a rescan mechanism to handle timing issues.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch parsed-text-race-condition

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

services/course-material/src/components/ParsedText/index.tsx (2)

53-63: Consider simplifying the mapping logic.

The flatMap with conditional array wrapping works correctly but could be more readable with a simple filter + map chain.

🔎 Alternative implementation:

 const scanGlossaryTargets = (container: HTMLElement | null): GlossaryTarget[] => {
   if (!container) {
     return []
   }
   const glossaryNodes = Array.from(container.querySelectorAll<HTMLElement>("[data-glossary-id]"))
 
-  return glossaryNodes.flatMap((node) => {
+  return glossaryNodes
+    .map((node) => {
-    const glossaryId = node.getAttribute("data-glossary-id")
-    return glossaryId ? [{ node, glossaryId }] : []
+      const glossaryId = node.getAttribute("data-glossary-id")
+      return glossaryId ? { node, glossaryId } : null
-  })
+    })
+    .filter((target): target is GlossaryTarget => target !== null)
 }

---

99-151: Consider making console warnings conditional for production.

The rescan logic is well-implemented with proper timer cleanup and state update optimization. However, the console.warn statements (lines 126-137) might be too verbose for production environments.

🔎 Consider making warnings conditional:

+const DEBUG = process.env.NODE_ENV === 'development'
+
 // ... inside the setTimeout callback
 
           if (containerChanged) {
+            if (DEBUG) {
             console.warn("ParsedText: Rescan needed - container changed", {
               delay,
               initialContainer,
               currentContainer,
             })
+            }
           }
           if (stale) {
+            if (DEBUG) {
             console.warn("ParsedText: Rescan needed - targets are stale", {
               delay,
               prevTargetsCount: prev.length,
             })
+            }
           }

Overall assessment: The implementation correctly handles timer cleanup, prevents unnecessary state updates via sameTargets, and uses function updaters to avoid race conditions. The multi-pass approach is a pragmatic workaround for DOM stability issues.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 525ba52 and b756471.

📒 Files selected for processing (1)

• services/course-material/src/components/ParsedText/index.tsx (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.tsx

⚙️ CodeRabbit configuration file

**/*.tsx: Make sure to use the useTranslation hook for all text that is visible to the user.
If a component uses colors, use colors from the theme if possible. Example: import { baseTheme } from "@/shared-module/common/styles" ... background: ${baseTheme.colors.clear[100]};
If a component uses media queries, use the respondToOrLarger function. Example: import { respondToOrLarger } from "@/shared-module/common/styles/respond" ... ${respondToOrLarger.sm} { ... }
If component sends requests to server, use either useQuery, or useToastMutation hooks, or use hooks wrapping those hooks.
If there is a form of any kind, make sure it uses useForm from react-hook-form, and make sure the form is integrated with the useToastMutation and useQuery hooks.
For loading and error states, use the Spinner and ErrorBanner components from the shared module.
We have better replacements for window.confirm, window.alert, and window.prompt, use the DialogProvider and the useDialog hook from the shared module instead.
For calling the React hooks useMemo and useEffect, don't use the React. prefix.
If a component uses fonts, use fonts from the theme if possible. Example: import { primaryFont } from "@/shared-module/common/styles" ... font-family: ${primaryFont};

Files:

• services/course-material/src/components/ParsedText/index.tsx

⏰ 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). (3)

• GitHub Check: build-and-system-tests
• GitHub Check: code-style
• GitHub Check: headless_lms

🔇 Additional comments (4)

services/course-material/src/components/ParsedText/index.tsx (4)

65-75: LGTM!

The comparison logic correctly uses reference equality for DOM nodes and value equality for glossaryId strings. The length check optimization is a good practice.

---

77-82: LGTM!

The validation logic correctly checks both isConnected (node in document) and container.contains(node) (node in specific container). The null container case is also handled appropriately.

---

95-97: LGTM!

State initialization and ref usage are correct. Using a ref to track timer IDs prevents unnecessary re-renders and enables proper cleanup.

---

50-51: Clarify or correct the React issue reference.

React issue #31600 relates to a regression when using createPortal with DOM elements created by dangerouslySetInnerHTML, which does not appear to match the glossary scanning use case. The RESCAN_DELAYS_MS workaround pattern—using multiple delayed rescans (0, 50, 200 ms)—suggests the code is addressing a different DOM stability or timing issue. Either update the comment to reference the correct React issue, or provide additional context about how #31600 specifically impacts glossary target scanning in this component.