Optimize GlossaryTable: pre-compute sanitized HTML #73
Conversation
|
🚀 Preview deployed to https://robotframework-RFCP-syllabus-pr-73.surge.sh/robotframework-RFCP-syllabus/ |
There was a problem hiding this comment.
Pull request overview
This PR optimizes the GlossaryTable component by moving markdown sanitization out of the render loop. Previously, sanitizeMarkdown was called for every filtered entry on every render, causing repeated markdown parsing and HTML sanitization. The optimization pre-computes the sanitized HTML once when creating display entries and stores it in a new definitionHtml field.
Key Changes:
- Added
definitionHtmlproperty to DisplayEntry type to store pre-computed sanitized HTML - Pre-compute sanitized HTML in the entries useMemo hook instead of during each render
- Updated purify dependency in useMemo to ensure recomputation when purify instance changes
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
🚀 Preview deployed to https://robotframework-RFCP-syllabus-pr-73.surge.sh/robotframework-RFCP-syllabus/ |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [entries, setEntries] = useState<DisplayEntry[]>([]); | ||
| const [aliasToCanonicalSlug, setAliasToCanonicalSlug] = useState<Map<string, string>>(new Map()); | ||
| const purifyRef = useRef<typeof DOMPurify | null>(null); | ||
| const [purifyReady, setPurifyReady] = useState(false); |
There was a problem hiding this comment.
Converting entries and aliasToCanonicalSlug from derived values (via useMemo) to state variables increases complexity unnecessarily. These values are derived from static glossaryData and don't need to be state.
The added state management introduces multiple effects, race conditions, and extra re-renders. The original useMemo approach was simpler and more appropriate since these values are computed synchronously from static data and can be safely computed during render.
There was a problem hiding this comment.
Already addressed in commit 1af6d50. I reverted to the useMemo approach, removing the unnecessary state variables (entries, aliasToCanonicalSlug) and useEffect hooks. The values are now computed synchronously from static glossaryData as derived values.
| useEffect(() => { | ||
| if (!purifyReady || !purifyRef.current) return; | ||
| setEntries((prev) => | ||
| prev.map((entry) => { | ||
| if (entry.isAlias || !entry.definitionHtml) return entry; | ||
| return { ...entry, definitionHtml: purifyRef.current!.sanitize(entry.definitionHtml) }; | ||
| }) | ||
| ); | ||
| }, [purifyReady]); |
There was a problem hiding this comment.
This useEffect creates a race condition and causes redundant state updates. It runs after purifyReady becomes true and updates all entries' definitionHtml fields. However, if this effect runs before the first useEffect (lines 50-94) completes, it will process an empty array. Even when it runs in the correct order, it triggers an unnecessary re-render by calling setEntries again after entries were just set.
The proper fix is to incorporate sanitization directly into the first useEffect (lines 50-94) by checking typeof window !== 'undefined' to determine if DOMPurify is available, matching the original implementation's approach.
There was a problem hiding this comment.
Already addressed in commit 1af6d50. I removed the problematic useEffect that was causing race conditions and redundant state updates. Sanitization now happens directly in the useMemo via the sanitizeMarkdown function which checks typeof window !== 'undefined' inline.
|
🚀 Preview deployed to https://robotframework-RFCP-syllabus-pr-73.surge.sh/robotframework-RFCP-syllabus/ |
|
🚀 Preview deployed to https://robotframework-RFCP-syllabus-pr-73.surge.sh/robotframework-RFCP-syllabus/ |
- Added `definitionHtml` field to DisplayEntry type - Compute sanitized HTML once when creating entries in useMemo - Use pre-computed HTML in render instead of calling sanitizeMarkdown on every render - This avoids re-parsing and re-sanitizing markdown on every render for every filtered entry Co-authored-by: Snooz82 <41592183+Snooz82@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Removed unnecessary state variables (entries, aliasToCanonicalSlug, purifyRef, purifyReady) - Reverted to useMemo for derived values from static glossaryData - Removed redundant useEffects that caused race conditions and extra re-renders - Simplified sanitizeMarkdown function to check window directly - Maintains security by sanitizing on client, unsanitized on SSR (original behavior) Co-authored-by: Snooz82 <41592183+Snooz82@users.noreply.github.com>
- Removed accidentally committed TypeScript build artifacts (.js files) - Updated .gitignore to exclude compiled JS files from TS sources - Kept legitimate source .js files (quizPrebuild.js, remark plugins) Co-authored-by: Snooz82 <41592183+Snooz82@users.noreply.github.com>
Snooz82
force-pushed
the
copilot/sub-pr-72
branch
from
56c4315 to
061abff
Compare
|
🚀 Preview deployed to https://robotframework-RFCP-syllabus-pr-73.surge.sh/robotframework-RFCP-syllabus/ |
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.