fix: address review comments on PR #447#480
Closed
narutamaaurum wants to merge 3 commits into
Closed
Conversation
…bles - terminal.tsx: Remove unnecessary eslint-disable comments (deps already correct) - playground-editor.tsx: Wrap updateEditorLanguage in useCallback - code-line.tsx: Move highlight/highlightCode outside component body Closes piyushdotcomm#442
- Fix missing React import: add useCallback to named imports in playground-editor.tsx - Remove redundant variable assignment in code-line.tsx - Add HTML escaping to prevent XSS in dangerouslySetInnerHTML usage
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
👋 Thanks for opening a PR, @narutamaaurum!Your PR has entered the 🚦 PR Review Pipeline.
What happens next
A pipeline status comment will appear below and update automatically as your PR progresses. While you wait
This comment is posted only once. |
WalkthroughRemoves eslint exhaustive-deps suppressions across three modules: extracts syntax-highlighting helpers in CodeLine, memoizes updateEditorLanguage in playground-editor, and writes the terminal initial prompt during initialization while cleaning up hook-lint suppressions. ChangesReact Hook Dependencies and Exhaustive-Deps Suppressions
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
modules/home/code-line.tsx (1)
31-36: ⚡ Quick winAvoid derived state for synchronous highlighting.
highlightedis fully derived fromline;useState+useEffectadds an extra render perCodeLineinstance.Recommended refactor
export const CodeLine = ({ line }: { line: string }) => { - const [highlighted, setHighlighted] = React.useState<React.ReactNode>(line); - - React.useEffect(() => { - setHighlighted(highlight(line)); - }, [line]); - - return highlighted; + const highlighted = React.useMemo(() => highlight(line), [line]); + return highlighted; };🤖 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 `@modules/home/code-line.tsx` around lines 31 - 36, The component CodeLine currently stores derived state highlighted (via React.useState and React.useEffect) even though highlighted is a pure synchronous transformation of the prop line (using highlight), causing an extra render; remove the state and effect and compute highlighted directly (or use React.useMemo(() => highlight(line), [line])) inside CodeLine so the UI renders once and still updates when line changes, referencing the highlighted variable, the highlight function, and the line prop for where to apply the change.
🤖 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 `@modules/home/code-line.tsx`:
- Around line 24-26: The comment parsing in the code-line.tsx branch uses
text.split('//') but only renders parts[0] and parts[1], dropping any additional
segments; update the rendering in the if (text.includes('//')) branch so that
after splitting you reconstruct the full comment by joining
parts.slice(1).join('//') (prefixing with '//' as before) and render
highlightCode(parts[0]) for the code portion and the reconstructed comment
string for the comment span; reference the variables text, parts and the
function highlightCode to locate the change.
- Around line 5-11: The string token highlighting fails because escapeHtml
transforms quotes into HTML entities before highlightCode runs its string
regexes (/'[^']*'/g and /"[^"]*"/g), so those patterns never match; fix by
running the tokenizer/highlighter against the raw input (call highlightCode on
the original text before calling escapeHtml) or by applying the string regexes
to the unescaped text and only escaping content when wrapping tokens for output;
update the flow in functions escapeHtml and highlightCode so string matching
happens on unescaped text and ensure any inserted HTML is escaped when you build
the final markup.
---
Nitpick comments:
In `@modules/home/code-line.tsx`:
- Around line 31-36: The component CodeLine currently stores derived state
highlighted (via React.useState and React.useEffect) even though highlighted is
a pure synchronous transformation of the prop line (using highlight), causing an
extra render; remove the state and effect and compute highlighted directly (or
use React.useMemo(() => highlight(line), [line])) inside CodeLine so the UI
renders once and still updates when line changes, referencing the highlighted
variable, the highlight function, and the line prop for where to apply the
change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c37d1e67-fae7-4a1f-90b5-74aa1f366520
📒 Files selected for processing (3)
modules/home/code-line.tsxmodules/playground/components/playground-editor.tsxmodules/webcontainers/components/terminal.tsx
💤 Files with no reviewable changes (1)
- modules/webcontainers/components/terminal.tsx
Comment on lines
+5
to
+11
| const escapeHtml = (text: string) => { | ||
| return text | ||
| .replace(/&/g, "&") | ||
| .replace(/</g, "<") | ||
| .replace(/>/g, ">") | ||
| .replace(/"/g, """) | ||
| .replace(/'/g, "'"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the file in question
FILE="modules/home/code-line.tsx"
echo "=== File exists? ==="
ls -la "$FILE" || true
echo "=== Lines 1-120 (for context) ==="
nl -ba "$FILE" | sed -n '1,160p'
echo "=== Find escapeHtml / highlightCode / highlight / dangerouslySetInnerHTML usages ==="
rg -n "escapeHtml|highlightCode|highlight\(|dangerouslySetInnerHTML|split\\(.*//|useState|useEffect" "$FILE" || true
echo "=== Extract suspected string/token regexes (around where comment references lines 18-19) ==="
# Print a small window around line numbers referenced by the review comment.
nl -ba "$FILE" | sed -n '10,60p'
echo "=== If present, inspect any string regex patterns ==="
rg -n --hidden --no-ignore -S "['\"]|string|literal|regex|match|replace\\(\\/.*\\/" "$FILE" || trueRepository: piyushdotcomm/Editron
Length of output: 258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="modules/home/code-line.tsx"
echo "=== File exists? ==="
ls -la "$FILE" || true
echo "=== Lines 1-180 (context) ==="
cat -n "$FILE" | sed -n '1,180p'
echo "=== Find key symbols/usages ==="
rg -n "escapeHtml|highlightCode|highlight\\(|dangerouslySetInnerHTML|split\\(|useState|useEffect" "$FILE" || true
echo "=== Extract escapeHtml function body (best-effort) ==="
# Print around the first occurrence of escapeHtml
LINE=$(rg -n "escapeHtml" "$FILE" | head -n 1 | cut -d: -f1 || true)
if [[ -n "${LINE:-}" ]]; then
START=$((LINE-10))
END=$((LINE+40))
(( START<1 )) && START=1
cat -n "$FILE" | sed -n "${START},${END}p"
fi
echo "=== Extract highlightCode/highlight usage sections ==="
rg -n "highlightCode|highlight\\(" "$FILE" || true
for pat in "highlightCode" "highlight(" "dangerouslySetInnerHTML" "split\\(" "useEffect" "useState"; do
echo "=== Around first match of: $pat ==="
LINE=$(rg -n "$pat" "$FILE" | head -n 1 | cut -d: -f1 || true)
if [[ -z "${LINE:-}" ]]; then
echo "No match for $pat"
continue
fi
START=$((LINE-15))
END=$((LINE+60))
(( START<1 )) && START=1
(( END>9999 )) && END=9999
cat -n "$FILE" | sed -n "${START},${END}p"
doneRepository: piyushdotcomm/Editron
Length of output: 11559
Quote escaping breaks string token highlighting.
highlightCode escapes the input via escapeHtml (turning " into " and ' into ') before applying the string regexes (/'[^']*'/g and /"[^"]*"/g`), so quoted literals never match and don’t get highlighted.
Proposed fix
const escapeHtml = (text: string) => {
return text
.replace(/&/g, "&")
.replace(/</g, "<")
- .replace(/>/g, ">")
- .replace(/"/g, """)
- .replace(/'/g, "&`#039`;");
+ .replace(/>/g, ">");
};Also applies to: 18-19
🤖 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 `@modules/home/code-line.tsx` around lines 5 - 11, The string token
highlighting fails because escapeHtml transforms quotes into HTML entities
before highlightCode runs its string regexes (/'[^']*'/g and /"[^"]*"/g), so
those patterns never match; fix by running the tokenizer/highlighter against the
raw input (call highlightCode on the original text before calling escapeHtml) or
by applying the string regexes to the unescaped text and only escaping content
when wrapping tokens for output; update the flow in functions escapeHtml and
highlightCode so string matching happens on unescaped text and ensure any
inserted HTML is escaped when you build the final markup.
Comment on lines
+24
to
+26
| if (text.includes('//')) { | ||
| const parts = text.split('//'); | ||
| return <><span dangerouslySetInnerHTML={{ __html: highlightCode(parts[0]) }} /><span className="text-slate-500 italic">{'//' + parts[1]}</span></>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and display the relevant portion of the file
FILE="modules/home/code-line.tsx"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
echo "---- $FILE (lines 1-120) ----"
nl -ba "$FILE" | sed -n '1,120p'
echo "---- $FILE (lines 18-40) ----"
nl -ba "$FILE" | sed -n '18,40p'
echo "---- $FILE: find split('//') usage ----"
rg -n "split\(['\"]//['\"]\)" "$FILE" || trueRepository: piyushdotcomm/Editron
Length of output: 178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="modules/home/code-line.tsx"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
echo "---- $FILE (lines 1-140) ----"
awk 'NR>=1 && NR<=140 { printf "%6d\t%s\n", NR, $0 }' "$FILE"
echo "---- $FILE: find split('//') usage ----"
rg -n "split\(['\"]//['\"]\)" "$FILE" || true
echo "---- $FILE: find text.split usages ----"
rg -n "text\.split\(" "$FILE" || trueRepository: piyushdotcomm/Editron
Length of output: 1994
Fix highlight() dropping content after additional //
text.split('//') is used, but only parts[0] and parts[1] are rendered ('//' + parts[1]), so any subsequent // segments (e.g., parts[2+]) are lost.
Proposed fix
const highlight = (text: string) => {
if (text.includes('//')) {
- const parts = text.split('//');
- return <><span dangerouslySetInnerHTML={{ __html: highlightCode(parts[0]) }} /><span className="text-slate-500 italic">{'//' + parts[1]}</span></>;
+ const commentStart = text.indexOf('//');
+ const codePart = text.slice(0, commentStart);
+ const commentPart = text.slice(commentStart);
+ return (
+ <>
+ <span dangerouslySetInnerHTML={{ __html: highlightCode(codePart) }} />
+ <span className="text-slate-500 italic">{commentPart}</span>
+ </>
+ );
}📝 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.
Suggested change
| if (text.includes('//')) { | |
| const parts = text.split('//'); | |
| return <><span dangerouslySetInnerHTML={{ __html: highlightCode(parts[0]) }} /><span className="text-slate-500 italic">{'//' + parts[1]}</span></>; | |
| if (text.includes('//')) { | |
| const commentStart = text.indexOf('//'); | |
| const codePart = text.slice(0, commentStart); | |
| const commentPart = text.slice(commentStart); | |
| return ( | |
| <> | |
| <span dangerouslySetInnerHTML={{ __html: highlightCode(codePart) }} /> | |
| <span className="text-slate-500 italic">{commentPart}</span> | |
| </> | |
| ); | |
| } |
🧰 Tools
🪛 ast-grep (0.43.0)
[warning] 25-25: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(react-unsafe-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 `@modules/home/code-line.tsx` around lines 24 - 26, The comment parsing in the
code-line.tsx branch uses text.split('//') but only renders parts[0] and
parts[1], dropping any additional segments; update the rendering in the if
(text.includes('//')) branch so that after splitting you reconstruct the full
comment by joining parts.slice(1).join('//') (prefixing with '//' as before) and
render highlightCode(parts[0]) for the code portion and the reconstructed
comment string for the comment span; reference the variables text, parts and the
function highlightCode to locate the change.
…odeLine Addresses CodeRabbit nitpick: highlighted is a pure synchronous transformation of the line prop, so useMemo avoids an extra render per CodeLine instance.
Author
|
Addressed the CodeRabbit nitpick: replaced + with for the derived state in component. This avoids an extra render per instance since is a pure synchronous transformation of the prop. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
modules/home/code-line.tsx (2)
24-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winComment parsing drops content after additional
//.
text.split('//')on line 25 produces an array, but line 26 only rendersparts[0]andparts[1], discardingparts[2+]. For example,const url = "http://example.com"; // commentsplits into three parts, but the final segment (comment) is lost.🔧 Proposed fix: use indexOf to split at the first `//`
const highlight = (text: string) => { if (text.includes('//')) { - const parts = text.split('//'); - return <><span dangerouslySetInnerHTML={{ __html: highlightCode(parts[0]) }} /><span className="text-slate-500 italic">{'//' + parts[1]}</span></>; + const commentStart = text.indexOf('//'); + const codePart = text.slice(0, commentStart); + const commentPart = text.slice(commentStart); + return ( + <> + <span dangerouslySetInnerHTML={{ __html: highlightCode(codePart) }} /> + <span className="text-slate-500 italic">{commentPart}</span> + </> + ); }🤖 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 `@modules/home/code-line.tsx` around lines 24 - 26, The current logic in the component handling `text` uses `text.split('//')` and renders only `parts[0]` and `parts[1]`, which discards any later segments; instead locate the first comment marker with `indexOf('//')`, split once into `code` and `comment`, then render `highlightCode(code)` and the full comment string (prefixed with `//`) so multi-`//` strings like URLs are preserved; update the code that defines `parts` and the returned JSX (references: variable `text`, the `parts` usage, and `highlightCode`) to use that single-index split approach.
14-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winQuote escaping breaks string token highlighting.
highlightCodecallsescapeHtmlon line 15, transforming"into"and'into'before applying the string-literal regexes on lines 18-19. The patterns/'[^']*'/gand/"[^"]*"/gexpect literal quote characters and will never match the escaped entities, so string literals remain unhighlighted.🔧 Proposed fix: apply string regexes before escaping
Refactor
highlightCodeto tokenize/wrap strings on the raw input, then escape the final assembled HTML:const highlightCode = (code: string) => { - const escaped = escapeHtml(code); - return escaped + return code .replace(/import|from|export|default|return|const|new/g, '<span class="text-red-500 dark:text-red-400 font-semibold">$&</span>') .replace(/'[^']*'/g, '<span class="text-amber-600 dark:text-amber-400">$&</span>') .replace(/"[^"]*"/g, '<span class="text-amber-600 dark:text-amber-400">$&</span>') - .replace(/Editron|console|editor/g, '<span class="text-rose-600 dark:text-rose-400">$&</span>'); + .replace(/Editron|console|editor/g, '<span class="text-rose-600 dark:text-rose-400">$&</span>') + .replace(/[&<>]/g, (char) => { + const entities: Record<string, string> = { '&': '&', '<': '<', '>': '>' }; + return entities[char] || char; + }); };This escapes
&<>at the end (after wrapping tokens) while leaving quotes intact for the regexes to match. Note: quotes inside the matched token text do not need escaping because they will be rendered as text content within the<span>tags inserted bydangerouslySetInnerHTML.🤖 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 `@modules/home/code-line.tsx` around lines 14 - 21, highlightCode currently escapes quotes before matching string regexes so string literals never match; fix by performing the string literal and token wrapping on the raw code first, then escaping the rest. Concretely: in highlightCode, run the /'[^']*'/ and /"[^"]*"/ and identifier/keyword replacements (and any other token replacements like /Editron|console|editor/ and the keyword regex) on the original code but emit unique placeholders (e.g. __SPAN_0__, __SPAN_1__) for each wrapped span, then call escapeHtml on the placeholder-filled text to safely escape &,<,> and finally replace the placeholders with their corresponding <span> HTML fragments. This preserves quote characters for matching and prevents escaping the injected span tags; refer to highlightCode and escapeHtml when making the change.
🤖 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.
Duplicate comments:
In `@modules/home/code-line.tsx`:
- Around line 24-26: The current logic in the component handling `text` uses
`text.split('//')` and renders only `parts[0]` and `parts[1]`, which discards
any later segments; instead locate the first comment marker with
`indexOf('//')`, split once into `code` and `comment`, then render
`highlightCode(code)` and the full comment string (prefixed with `//`) so
multi-`//` strings like URLs are preserved; update the code that defines `parts`
and the returned JSX (references: variable `text`, the `parts` usage, and
`highlightCode`) to use that single-index split approach.
- Around line 14-21: highlightCode currently escapes quotes before matching
string regexes so string literals never match; fix by performing the string
literal and token wrapping on the raw code first, then escaping the rest.
Concretely: in highlightCode, run the /'[^']*'/ and /"[^"]*"/ and
identifier/keyword replacements (and any other token replacements like
/Editron|console|editor/ and the keyword regex) on the original code but emit
unique placeholders (e.g. __SPAN_0__, __SPAN_1__) for each wrapped span, then
call escapeHtml on the placeholder-filled text to safely escape &,<,> and
finally replace the placeholders with their corresponding <span> HTML fragments.
This preserves quote characters for matching and prevents escaping the injected
span tags; refer to highlightCode and escapeHtml when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: db357689-9840-4513-a31a-76cb1d082688
📒 Files selected for processing (1)
modules/home/code-line.tsx
Author
|
Superseded by #481 which addresses the remaining CodeRabbit review comments. Closing this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses the review comments from CodeRabbit and Qodo on the original PR #447:
Fixes
Fixed missing React import in playground-editor.tsx (Critical bug from Qodo)
Removed redundant variable assignment in code-line.tsx (CodeRabbit nitpick)
Added HTML escaping to prevent XSS (CodeRabbit security concern)
Testing
Closes #442
Summary by CodeRabbit