fix: address CodeRabbit review comments on code-line.tsx#481
fix: address CodeRabbit review comments on code-line.tsx#481narutamaaurum wants to merge 5 commits into
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
…odeLine Addresses CodeRabbit nitpick: highlighted is a pure synchronous transformation of the line prop, so useMemo avoids an extra render per CodeLine instance.
- Fix comment parsing to preserve content after additional '//' by using indexOf instead of split - Fix string token highlighting by applying regexes on raw code before escaping HTML entities - Use placeholder approach to preserve span tags during escaping
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThree React components updated to remove eslint exhaustive-deps suppressions: code-line highlighting replaced with a pure memoized highlighter, playground editor language updater memoized with useCallback, and terminal component lint-suppression comments removed. ChangesReact Hook Dependencies
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
This PR addresses the remaining CodeRabbit review comments on the original PR #447 (now closed in favor of this PR): Changes in this PR1. Fixed comment parsing to preserve content after additionalFile:
2. Fixed string token highlightingFile: ( function)
The fix for derived state (replacing + ) was already applied in the previous commit. This resolves the actionable feedback from CodeRabbit review. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
modules/playground/components/playground-editor.tsx (1)
269-271: 💤 Low valueOptional: simplify dependency array.
The dependency array includes both
activeFileandupdateEditorLanguage, but sinceupdateEditorLanguageis memoized with[activeFile]as its only dependency, includingactiveFilehere is redundant. The effect will run whenactiveFilechanges (viaupdateEditorLanguagechanging).Consider simplifying to:
♻️ Optional simplification
useEffect(() => { updateEditorLanguage(); - }, [activeFile, updateEditorLanguage]); + }, [updateEditorLanguage]);🤖 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/playground/components/playground-editor.tsx` around lines 269 - 271, The effect's dependency array redundantly includes both activeFile and updateEditorLanguage; since updateEditorLanguage is memoized with activeFile as its sole dependency, remove activeFile from the useEffect dependency list so it only depends on updateEditorLanguage (i.e., keep useEffect(() => { updateEditorLanguage(); }, [updateEditorLanguage])); reference useEffect, updateEditorLanguage, and activeFile to locate and update the call.
🤖 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`:
- Line 28: The regex used in the .replace(...) call on the code line (the call
that invokes replaceWithPlaceholder) lacks word boundaries so it matches
substrings like "important" or "newfound"; update the pattern to use word
boundaries and a non-capturing group, e.g. change the pattern to include \b
around the keywords (/\b(?:import|from|export|default|return|const|new)\b/g) so
only whole keywords are matched while still calling replaceWithPlaceholder as
before.
- Around line 27-31: The matched token content is being interpolated directly
into span templates causing potential XSS; update the replace callbacks that
build `highlighted` (the calls to `replaceWithPlaceholder`) to HTML-escape the
matched text before inserting it into the span (use the existing `escapeHtml`
helper) so you embed safe text, e.g. call `escapeHtml(match)` when constructing
the inner content for the spans in the `replace` handlers for keywords,
single/double-quoted strings, and identifiers (`Editron|console|editor`) instead
of inserting `match` raw.
---
Nitpick comments:
In `@modules/playground/components/playground-editor.tsx`:
- Around line 269-271: The effect's dependency array redundantly includes both
activeFile and updateEditorLanguage; since updateEditorLanguage is memoized with
activeFile as its sole dependency, remove activeFile from the useEffect
dependency list so it only depends on updateEditorLanguage (i.e., keep
useEffect(() => { updateEditorLanguage(); }, [updateEditorLanguage])); reference
useEffect, updateEditorLanguage, and activeFile to locate and update the call.
🪄 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: b6f3c855-a536-419c-9d55-68b886473a07
📒 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
…nd-editor.tsx - Add word boundaries to regex patterns in code-line.tsx to prevent substring matching - Escape matched text before inserting into spans to prevent XSS - Simplify useEffect dependency array in playground-editor.tsx (remove redundant activeFile) Closes piyushdotcomm#481
|
Addressed the CodeRabbit review comments:
All changes are minimal and targeted to address the specific concerns raised. |
|
Created PR #490 with the word boundaries fix for the CodeRabbit review comments on code-line.tsx. |
|
Created PR #491 with the useEffect dependency fix for the CodeRabbit review comments on playground-editor.tsx. |
This PR addresses the remaining CodeRabbit review comments on the original PR #447:
Fixes
**Fixed comment parsing in ** (CodeRabbit nitpick)
**Fixed string token highlighting in ** (CodeRabbit major issue)
The fix for derived state was already applied in the previous commit.
Closes #442
Summary by CodeRabbit
Refactor
Chores