Knowledge Center formatting issues#1147
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a decodeHtmlEntities utility and applies it to titles, descriptions, and author/channel/provider fields across loaders, viewers, dashboard lists, and card components; also tweaks card flex layouts and several Tailwind sizing classes. ChangesHTML Entity Decoding Across Knowledge Center and Dashboard Pages
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/lib/decodeHtmlEntities.ts`:
- Around line 1-6: The static analyzer flags the innerHTML assignment in
decodeHtmlEntities as a potential XSS false positive; to resolve this either (A)
replace the textarea trick with DOMParser: in decodeHtmlEntities keep the early
return for falsy input, create a DOMParser, call parseFromString(input,
'text/html') and return document.body.textContent (or textContent of the parsed
document) instead of using el.innerHTML/el.value, or (B) switch to a proven
library like `he`/`html-entities` and call its decode function while preserving
the same falsy-input behavior; alternatively add a short code comment above
decodeHtmlEntities explaining why the textarea approach is safe (detached
element, returning .value) to silence the warning if you want to keep the
current implementation.
In `@frontend/src/pages/knowledge-center/VideoViewer.tsx`:
- Around line 131-133: The JSX accesses video.duration directly which can be
undefined; update the rendering in VideoViewer.tsx to guard against a missing
video by using optional chaining or a null check (e.g., refer to the video
variable and replace video.duration with video?.duration or wrap the whole
expression in a conditional that checks video), ensuring formatVideoDuration is
only called when video is defined (e.g., call
formatVideoDuration(video.duration) only after confirming video exists).
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 72d548cb-723c-4c39-9169-91266008615f
📒 Files selected for processing (8)
frontend/src/components/dashboard/TopContentList.tsxfrontend/src/lib/decodeHtmlEntities.tsfrontend/src/loaders/routeLoaders.tsfrontend/src/pages/knowledge-center/KnowledgeCenterManagement.tsxfrontend/src/pages/knowledge-center/LibraryViewer.tsxfrontend/src/pages/knowledge-center/ResidentKnowledgeCenter.tsxfrontend/src/pages/knowledge-center/VideoViewer.tsxfrontend/src/pages/student/ResidentHome.tsx
| export function decodeHtmlEntities(input: string): string { | ||
| if (!input) return input; | ||
| const el = document.createElement('textarea'); | ||
| el.innerHTML = input; | ||
| return el.value; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Static analysis warnings are false positives for this entity-decoding pattern.
The static analysis tools flagged innerHTML assignment as an XSS risk. However, this specific pattern is safe because:
- The
textareaelement is created in memory and never appended to the DOM - Scripts cannot execute in a detached element
- The function returns
.value(decoded text), not.innerHTML(HTML content) - This is a standard lightweight technique for HTML entity decoding
That said, if you want to address the warnings explicitly or improve code clarity, consider these alternatives:
Alternative approaches
Option 1: Use DOMParser (more explicit intent, still native)
export function decodeHtmlEntities(input: string): string {
if (!input) return input;
- const el = document.createElement('textarea');
- el.innerHTML = input;
- return el.value;
+ const doc = new DOMParser().parseFromString(input, 'text/html');
+ return doc.documentElement.textContent || input;
}Option 2: Use a dedicated library like he or html-entities
import { decode } from 'he';
export function decodeHtmlEntities(input: string): string {
if (!input) return input;
return decode(input);
}Libraries handle edge cases and numeric entities more robustly, but add a dependency.
The current implementation is correct and safe for the stated use case (decoding provider-sourced content like "Q&A" → "Q&A").
🧰 Tools
🪛 ast-grep (0.43.0)
[warning] 3-3: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: el.innerHTML = input
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 3-3: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: el.innerHTML = input
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🪛 OpenGrep (1.22.0)
[WARNING] 4-4: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/lib/decodeHtmlEntities.ts` around lines 1 - 6, The static
analyzer flags the innerHTML assignment in decodeHtmlEntities as a potential XSS
false positive; to resolve this either (A) replace the textarea trick with
DOMParser: in decodeHtmlEntities keep the early return for falsy input, create a
DOMParser, call parseFromString(input, 'text/html') and return
document.body.textContent (or textContent of the parsed document) instead of
using el.innerHTML/el.value, or (B) switch to a proven library like
`he`/`html-entities` and call its decode function while preserving the same
falsy-input behavior; alternatively add a short code comment above
decodeHtmlEntities explaining why the textarea approach is safe (detached
element, returning .value) to silence the warning if you want to keep the
current implementation.
carddev81
left a comment
carddev81
left a comment
There was a problem hiding this comment.
Tests good and looks good. I did find other issues that I made notes of. I'm creating other tickets for these.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/pages/knowledge-center/KnowledgeCenterManagement.tsx`:
- Around line 553-562: The toast messages use the same actionString for both
success and failure, causing error branches to display misleading success-like
text; update the error branch to use a failure message (e.g., "failed to
[feature/unfeature]") instead of actionString and include any error info from
resp (or resp.error) when available; locate the block using actionString,
isFeatured, API.put, resp, toast.success/toast.error and change the toast.error
call to display a clear failure message referencing typeLabel[type] and the
inverse verb (e.g., `failed to ${actionString}`) and preferably append
resp.error or resp.message for debugging, and apply the same change to the
similar block that calls mutateLibs/mutateVids/mutateLinks.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: e107a713-523b-4aac-a6c9-9c583432bb66
📒 Files selected for processing (3)
frontend/src/pages/knowledge-center/KnowledgeCenterManagement.tsxfrontend/src/pages/knowledge-center/LibraryViewer.tsxfrontend/src/pages/knowledge-center/VideoViewer.tsx
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/pages/knowledge-center/KnowledgeCenterManagement.tsx`:
- Around line 553-562: The toast messages use the same actionString for both
success and failure, causing error branches to display misleading success-like
text; update the error branch to use a failure message (e.g., "failed to
[feature/unfeature]") instead of actionString and include any error info from
resp (or resp.error) when available; locate the block using actionString,
isFeatured, API.put, resp, toast.success/toast.error and change the toast.error
call to display a clear failure message referencing typeLabel[type] and the
inverse verb (e.g., `failed to ${actionString}`) and preferably append
resp.error or resp.message for debugging, and apply the same change to the
similar block that calls mutateLibs/mutateVids/mutateLinks.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: e107a713-523b-4aac-a6c9-9c583432bb66
📒 Files selected for processing (3)
frontend/src/pages/knowledge-center/KnowledgeCenterManagement.tsxfrontend/src/pages/knowledge-center/LibraryViewer.tsxfrontend/src/pages/knowledge-center/VideoViewer.tsx
🛑 Comments failed to post (1)
frontend/src/pages/knowledge-center/KnowledgeCenterManagement.tsx (1)
553-562:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError toasts currently report success-like outcomes.
On Line 561 and Line 589, failed API calls still render success-style phrases derived from
actionString, which can tell admins the update happened when it did not.Proposed fix
const actionString = isFeatured ? 'unfeatured' : 'featured'; const resp = await API.put<null, object>(endpoints[type], {}); if (resp.success) { toast.success(`${typeLabel[type]} ${actionString}`); if (type === 'library') void mutateLibs(); else if (type === 'video') void mutateVids(); else void mutateLinks(); } else { - toast.error(`${typeLabel[type]} ${actionString}`); + toast.error( + resp.message ?? + `Failed to update featured status for ${typeLabel[type].toLowerCase()}` + ); } @@ const actionString = isVisible ? 'is now hidden' : 'is now visible'; const resp = await API.put<null, object>(endpoints[type], {}); if (resp.success) { toast.success(`${typeLabel[type]} ${actionString}`); if (type === 'library') void mutateLibs(); else if (type === 'video') void mutateVids(); else void mutateLinks(); } else { - toast.error(`${typeLabel[type]} ${actionString}`); + toast.error( + resp.message ?? + `Failed to update visibility for ${typeLabel[type].toLowerCase()}` + ); }Also applies to: 581-590
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/knowledge-center/KnowledgeCenterManagement.tsx` around lines 553 - 562, The toast messages use the same actionString for both success and failure, causing error branches to display misleading success-like text; update the error branch to use a failure message (e.g., "failed to [feature/unfeature]") instead of actionString and include any error info from resp (or resp.error) when available; locate the block using actionString, isFeatured, API.put, resp, toast.success/toast.error and change the toast.error call to display a clear failure message referencing typeLabel[type] and the inverse verb (e.g., `failed to ${actionString}`) and preferably append resp.error or resp.message for debugging, and apply the same change to the similar block that calls mutateLibs/mutateVids/mutateLinks.
corypride
force-pushed
the
cpride/kc_formatting
branch
from
7aecd79 to
253d128
Compare
2 participants
fix: decode HTML entities in Knowledge Center + card formatting (ID-661)
Pre-Submission PR Checklist
Description of the change
Fixes two formatting issues in the Knowledge Center reported in ID-661:
HTML entities rendering literally — provider-sourced text (e.g. a library
titled
Cooking Q&A) was displaying the raw entity instead ofQ&A.Added a small
decodeHtmlEntitieshelper (frontend/src/lib/decodeHtmlEntities.ts)and applied it at every live render site that displays provider title /
description / channel / author / provider-name text, including
altattributes (React does not entity-decode JS strings, so
alt={title}showedthe literal
&too). The helper is entity-agnostic — it decodes any namedor numeric HTML entity, not just
&.Sites updated:
KnowledgeCenterManagement.tsx— Library/Video/Link admin cardsResidentKnowledgeCenter.tsx— resident content cards (covers search results)LibraryViewer.tsx/VideoViewer.tsx— viewer headers, descriptions, channelcomponents/dashboard/TopContentList.tsx— dashboard "Top Content" widgetpages/student/ResidentHome.tsx— featured libraries, helpful links, favoritesloaders/routeLoaders.ts— library filter dropdown option labelsCard styling — cards in a row had uneven heights and the
Visibletogglerow didn't align. Cards now use
flex flex-col h-fullwith aflex-1description and
mt-autoon the control row, giving equal-height cards withbottom-aligned toggles.
Also cleaned up Tailwind v4 canonical-class lint hints on the touched files
(
flex-shrink-0→shrink-0; arbitraryw-[…px]/h-[…px]/min-h-[2.5rem]→scale equivalents like
w-45,w-70,w-80,h-100,h-150,min-h-10).These are pixel-equivalent — no visual change.
Screenshot(s)
TODO: add before/after of the Knowledge Center cards (1366 x 768 for the
resident-facing views) showing
Q&Arendering correctly and aligned toggles.Additional context
the pattern already established on the management cards.
components/knowledge-center/(
LibraryCard,VideoCard,HelpfulLinkCard,FavoriteCard,OpenContentItemAccordion) were intentionally left untouched — a usage grepshows zero imports; they're dead code and out of scope here.
tsc --noEmitandeslintpass clean on all edited files.