fix: code quality and safety improvements - #913
Conversation
github-actions
Bot
added
gssoc
|
👋 Thank you @saurabhhhcodes for your contribution to Late-Meet!
Please review any automated suggestions or code review comments that may appear below! We will review your PR as soon as possible! Please consider starring the repository ⭐ to show your support! |
📝 WalkthroughWalkthroughThe dashboard now assigns loading, success, and error messages with ChangesDOM rendering
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
🚀 Thank You for Contributing to Late-MeetPlease ensure that:
Thank you for contributing 💙 |
|
![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 `@src/apiUsageDashboard.ts`:
- Around line 18-26: Update src/apiUsageDashboard.ts lines 18-26 to render the
loading and error paragraphs as DOM elements and insert
buildDashboardHTML(stats) through innerHTML or equivalent DOM construction,
ensuring dynamic values are escaped; update src/utils/domHelpers.ts line 29
likewise by creating the span through DOM APIs or using innerHTML with
escapeHtml().
In `@src/utils/domHelpers.ts`:
- Line 29: Correct the usage example in the escapeHtml documentation comment:
use raw user.displayName with textContent, or use escapeHtml(user.displayName)
with innerHTML when demonstrating escaped HTML. Do not combine HTML markup with
textContent, since it remains literal text.
🪄 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 Plus
Run ID: 16c83056-0592-42d8-9974-cabe1235ef83
📒 Files selected for processing (2)
src/apiUsageDashboard.tssrc/utils/domHelpers.ts
| container.textContent = '<p class="usage-loading">Loading API usage stats…</p>'; | ||
|
|
||
| try { | ||
| const stats = await getUsageStats(); | ||
| container.innerHTML = buildDashboardHTML(stats); | ||
| container.textContent = buildDashboardHTML(stats); | ||
| attachEventListeners(container); | ||
| } catch (err) { | ||
| console.error("[LateMeet] Failed to load API usage dashboard:", err); | ||
| container.innerHTML = '<p class="usage-error">Failed to load API usage data.</p>'; | ||
| container.textContent = '<p class="usage-error">Failed to load API usage data.</p>'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files | rg '(^|/)apiUsageDashboard\.ts$|(^|/)domHelpers\.ts$' || true
printf '\napiUsageDashboard outline:\n'
ast-grep outline src/apiUsageDashboard.ts || true
printf '\ndomHelpers outline:\n'
ast-grep outline src/utils/domHelpers.ts || true
printf '\nRelevant snippets:\n'
sed -n '1,120p' src/apiUsageDashboard.ts
printf '\n---\n'
sed -n '1,120p' src/utils/domHelpers.ts
printf '\nSearch for helper/dashboard functions:\n'
rg -n "buildDashboardHTML|getUsageStats|escapeHtml|attachEventListeners|usage-loading|usage-error" src || trueRepository: shouri123/Late-Meet
Length of output: 14426
🏁 Script executed:
# Read-only behavioral probe: show exactly what text node content each current assignment produces,
# without executing repository code or modifying files.
node - <<'JS'
const samples = [
['loading', '<p class="usage-loading">Loading API usage stats…</p>'],
['error', '<p class="usage-error">Failed to load API usage data.</p>'],
['example usage', '<div class="usage-dashboard"><div class="usage-card"><span>Tokens</span><strong>42</strong></div></div>'],
['example dynamic with angle brace', '<span>Alice <script>alert(1)</script> Smith</span>'],
];
for (const [name, html] of samples) {
const doc = { body: document.createElement('div') };
const container = doc.body;
// Simulate current .textContent assignment.
container.textContent = html;
console.log(`${name}:`);
console.log('textContent produces a single text node containing:', JSON.stringify(container.textContent));
console.log('outerHTML:', container.outerHTML);
console.log('firstChild tagName:', container.firstChild?.tagName ?? '<text>');
console.log('has usage-card class:', container.classList && container.classList.contains('usage-card'));
console.log('has script element:', container.querySelector?.('script') != null);
console.log();
}
JSRepository: shouri123/Late-Meet
Length of output: 793
🌐 Web query:
MDN Node textContent string markup rendered as text vs innerHTML
💡 Result:
The difference between using Node.textContent and Element.innerHTML primarily lies in how the browser handles markup, security, and performance [1][2]. Node.textContent - Rendering: When you set textContent, the browser treats the input string as raw text [1][2]. Any HTML markup included in the string (e.g., hello) will not be rendered as HTML; instead, it will appear literally as text on the page [2][3]. - Security: It is secure against cross-site scripting (XSS) attacks because it does not parse or execute the provided string as HTML [1][4]. - Performance: It is generally faster than innerHTML because the browser does not need to invoke the HTML parser [1][4]. Element.innerHTML - Rendering: When you set innerHTML, the browser parses the provided string as HTML markup [2]. Any valid HTML tags within the string will be rendered according to their standard behaviors [2][3]. - Security: It is a common vector for XSS attacks if the content being inserted comes from an untrusted source, as malicious scripts could be injected and executed [2][4]. - Performance: It is slower than textContent because it must invoke the HTML parser to construct the DOM tree from the provided string [1][4]. In summary, MDN advises using textContent whenever you intend to display plain text, as it is more semantic, performant, and secure [1][4]. You should only use innerHTML when you specifically need to insert and render HTML markup, and even then, you should ensure the content is sanitized or trusted to prevent security vulnerabilities [2][5].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Element/innerHTML
- 3: https://www.freecodecamp.org/news/innerhtml-vs-innertext-vs-textcontent/
- 4: https://github.com/mdn/content/blob/main/files/en-us/web/api/node/textcontent/index.md
- 5: https://stackoverflow.com/questions/24427621/innertext-vs-innerhtml-vs-label-vs-text-vs-textcontent-vs-outertext
Do not assign HTML markup to textContent.
textContent treats the string as raw text, so the dashboard’s loading, error, and generated markup are rendered literally instead of as elements. Render the loading/error <p> elements explicitly, and insert buildDashboardHTML(stats) via innerHTML or DOM construction with escaped dynamic values. The src/utils/domHelpers.ts example also has this issue; create the <span> in the DOM or use innerHTML with escapeHtml().
📍 Affects 2 files
src/apiUsageDashboard.ts#L18-L26(this comment)src/utils/domHelpers.ts#L29-L29
🤖 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 `@src/apiUsageDashboard.ts` around lines 18 - 26, Update
src/apiUsageDashboard.ts lines 18-26 to render the loading and error paragraphs
as DOM elements and insert buildDashboardHTML(stats) through innerHTML or
equivalent DOM construction, ensuring dynamic values are escaped; update
src/utils/domHelpers.ts line 29 likewise by creating the span through DOM APIs
or using innerHTML with escapeHtml().
| * | ||
| * @example | ||
| * element.innerHTML = `<span>${escapeHtml(user.displayName)}</span>`; | ||
| * element.textContent = `<span>${escapeHtml(user.displayName)}</span>`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline src/utils/domHelpers.ts || true
echo "== relevant file contents =="
cat -n src/utils/domHelpers.ts | sed -n '1,160p'
echo "== references to domHelpers/escapeHtml =="
rg -n "escapeHtml|domHelpers|textContent|innerHTML" src -S || true
echo "== local script behavior probe for TextContent/InnerHTML semantics in browser would require runtime; instead use Node browserless unavailable by requirement. We'll inspect only source/config."Repository: shouri123/Late-Meet
Length of output: 25216
🌐 Web query:
MDN textContent property HTML parsing does not parse tags
💡 Result:
The textContent property does not parse HTML tags. When you read the textContent of a node, it returns the concatenated text content of that node and all its descendants, explicitly excluding any HTML tags [1][2]. When you set the textContent of a node, the provided string is treated as raw text rather than HTML markup; it does not invoke the HTML parser [1][3][4]. Consequently, any HTML tags included in the string will be rendered as literal text on the page rather than being interpreted by the browser [4]. This behavior makes textContent a secure way to insert user-provided content into the DOM, as it prevents cross-site scripting (XSS) attacks that could otherwise occur if the input were parsed as HTML [1][3]. In contrast, the innerHTML property does invoke the HTML parser, meaning it treats the input string as markup and converts it into DOM elements [3][4].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent
- 2: https://github.com/mdn/content/blob/main/files/en-us/web/api/node/textcontent/index.md
- 3: https://developer.mozilla.org/en-US/docs/Web/API/Element/innerHTML
- 4: https://stackoverflow.com/questions/21311299/nodevalue-vs-innerhtml-and-textcontent-how-to-choose
Correct the escapeHtml example.
textContent does not parse HTML. The example assigns literal <span>...</span> text instead of creating a span. Set the raw user.displayName as textContent, or assign escapeHtml(user.displayName) through innerHTML if the example is meant to demonstrate escaped HTML injection.
🤖 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 `@src/utils/domHelpers.ts` at line 29, Correct the usage example in the
escapeHtml documentation comment: use raw user.displayName with textContent, or
use escapeHtml(user.displayName) with innerHTML when demonstrating escaped HTML.
Do not combine HTML markup with textContent, since it remains literal text.
1 participant
Summary by CodeRabbit
Bug Fixes
Documentation