refactor: enhance React Scan detection and removal logic #139
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@react-grab/cli
grab
@react-grab/ami
@react-grab/amp
@react-grab/claude-code
@react-grab/codex
@react-grab/cursor
@react-grab/droid
@react-grab/gemini
@react-grab/opencode
react-grab
@react-grab/relay
commit: |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
| ); | ||
| } | ||
|
|
||
| const grouped = Map.groupBy(countByComponent, ([, count]) => count); |
There was a problem hiding this comment.
Critical: Browser Compatibility Issue
Map.groupBy() is an ES2024 feature with very limited browser support (not available in Safari, Firefox ESR, and older Chrome versions). This will cause runtime errors in unsupported browsers.
Recommended fix:
const grouped = new Map<number, Array<[string, number]>>();
for (const [name, count] of countByComponent) {
if (!grouped.has(count)) {
grouped.set(count, []);
}
grouped.get(count)!.push([name, count]);
}|
|
||
| const VITE_REMOVAL_PATTERNS: RegExp[] = [ | ||
| /\s*import\s*\(\s*["']react-scan["']\s*\);?/g, | ||
| /\s*import\s*\(\s*["']react-scan["']\s*\)\s*\.then\s*\([^)]*\)[^;]*;?/g, |
There was a problem hiding this comment.
Risk: Overly Greedy Pattern
The pattern [^)]* followed by [^;]* is too permissive and could accidentally match beyond the intended .then() call. For example:
import("react-scan").then(() => console.log("loaded")); doSomething();The [^;]* will match ) doSomething(), potentially removing user code after the import.
Consider making the pattern more specific:
/\s*import\s*\(\s*["']react-scan["']\s*\)\s*\.then\s*\([^)]*\)(?:\s*\.catch\s*\([^)]*\))?;?/g| const WEBPACK_REMOVAL_PATTERNS: RegExp[] = [ | ||
| /\s*import\s*\(\s*["']react-scan["']\s*\);?/g, | ||
| /^\s*import\s+(?:\{[^}]*\}|\*\s+as\s+\w+|\w+)\s+from\s+["']react-scan["'];?\s*$/gm, | ||
| /if\s*\(\s*process\.env\.NODE_ENV\s*===\s*["']development["']\s*\)\s*\{\s*\}/g, |
There was a problem hiding this comment.
Minor: Pattern May Not Match After First Removal
This pattern matches empty if blocks, but it relies on \s* to match any whitespace between { and }. After the first pattern removes the import statement, there could be varied whitespace (newlines, indentation) that should be matched. The pattern should work, but consider testing edge cases with different indentation styles.
Also, this pattern could theoretically match legitimate empty development conditionals unrelated to React Scan. Consider adding a comment in the code to document this behavior.
| const combinedOriginal = removalResult.originalContent!; | ||
| const combinedNew = addResult.newContent!; |
There was a problem hiding this comment.
Bug: Incorrect Preview Diff
When removal and addition happen in the same file, this shows removalResult.originalContent → addResult.newContent. However, addResult was calculated from the original file (line 275-281), not from the file after React Scan removal. This means addResult.newContent still contains React Scan code, making the preview diff misleading.
The actual application order (lines 389-401) is correct, so this is a display-only bug.
Fix:
let combinedNew = removalResult.newContent!;
// Need to recalculate addResult based on removed content
// Or chain the operations properlyAlternatively, you could apply the removal transform to addResult.newContent before showing the diff, or show both diffs separately even when they're in the same file.
| ]; | ||
|
|
||
| export const REACT_SCAN_DETECTION_PATTERNS: RegExp[] = [ | ||
| /["'`][^"'`]*react-scan/, |
There was a problem hiding this comment.
Good: Comprehensive Pattern Detection
The detection patterns cover multiple installation methods (CDN, npm, import, require). However, pattern /["'\][^"'\`]*react-scan/` could theoretically match "react-scan" in comments or strings unrelated to the actual library.
This is unlikely to cause issues in practice, but worth noting for future refinement if false positives occur.
|
|
||
| expect(result.success).toBe(true); | ||
| expect(result.noChanges).toBeFalsy(); | ||
| expect(result.newContent).not.toContain("react-scan"); |
There was a problem hiding this comment.
Test Gap: Missing Assertion
The test verifies react-scan is removed and React is preserved, but doesn't verify that the empty if block itself was also removed. Consider adding:
expect(result.newContent).not.toContain('if (process.env.NODE_ENV');|
|
||
| export const startRecording = (): void => { | ||
| if (!setupInstrumentation()) return; | ||
| renderLogHistory = []; |
There was a problem hiding this comment.
Minor: Potential Data Loss
startRecording() clears renderLogHistory unconditionally. If a user accidentally clicks record twice or the function is called programmatically multiple times, they lose their previous recording data without warning.
Consider either:
- Checking if there's existing data and prompting the user
- Not clearing the history automatically
- Documenting this behavior clearly in the UI
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Bugbot Autofix prepared fixes for 4 of the 4 bugs found in the latest run.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 17 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/cli/test/detect.test.ts">
<violation number="1" location="packages/cli/test/detect.test.ts:229">
P2: These tests use a global mock that returns `package.json` content for all file reads. Since `detectReactScan` scans multiple files (like `layout.tsx`, `index.html`) using regex patterns, and the mocked `package.json` contains "react-scan", the function will incorrectly "detect" `react-scan` in every checked file.
This creates a false positive scenario where `detectedFiles` is populated even when checking only dependencies, potentially masking bugs in the file detection logic.
Fix by strictly mocking `package.json` and other files separately.</violation>
</file>
<file name="packages/cli/src/utils/transform.ts">
<violation number="1" location="packages/cli/src/utils/transform.ts:1633">
P1: This regex strictly requires parentheses around the component in the conditional expression. If the code was formatted (e.g., by Prettier) to remove unnecessary parentheses, this pattern will fail to match.
However, the subsequent pattern `/\s*<Script[^>]*react-scan[^>]*\/>/gi` will likely succeed, removing the component but leaving the conditional wrapper `{process.env.NODE_ENV === 'development' && }`, which causes a syntax error.</violation>
</file>
<file name="packages/react-grab/src/components/toolbar/state.ts">
<violation number="1" location="packages/react-grab/src/components/toolbar/state.ts:12">
P2: Input `mode` from localStorage is not validated against the `ToolbarMode` union. If the stored JSON contains `null` (valid JSON) or an invalid string, it could be returned as-is, violating the type contract and potentially causing runtime errors in consumers.
Add validation to ensure only valid modes are returned.</violation>
</file>
<file name="packages/react-grab/src/components/icons/icon-play.tsx">
<violation number="1" location="packages/react-grab/src/components/icons/icon-play.tsx:3">
P2: This component deviates from the codebase's icon patterns (e.g., `IconCheck`, `IconChevron`):
1. Hardcoded `white` colors limit reusability (should use `currentColor`).
2. Lacks a `size` prop for scaling.
The fix aligns it with other icons and defaults to the current 6x7 size. Ensure usages (e.g., in `toolbar/index.tsx`) set a text color (like `text-white`) if relying on the previous hardcoded value.</violation>
</file>
<file name="packages/react-grab/src/components/render-scan.tsx">
<violation number="1" location="packages/react-grab/src/components/render-scan.tsx:83">
P1: `Map.groupBy` is not supported in Node.js 18 or older browsers (e.g. Chrome < 117, Safari < 17.4). Since the project supports Node 18 (`engines` or `dependencies`), this will cause runtime errors in supported environments. Use a manual grouping loop instead.</violation>
</file>
<file name="packages/react-grab/src/design-system.tsx">
<violation number="1" location="packages/react-grab/src/design-system.tsx:2577">
P2: The Play icon SVG is hardcoded here, duplicating the logic from `IconPlay`.
Use the existing `IconPlay` component (from `src/components/icons/icon-play.tsx`) to ensure consistency and reduce maintenance overhead. You will need to add `import { IconPlay } from "./components/icons/icon-play.jsx";` to the imports.</violation>
</file>
<file name="packages/cli/test/transform.test.ts">
<violation number="1" location="packages/cli/test/transform.test.ts:919">
P2: Tests do not cover valid JSX conditionals without parentheses (e.g., `&& <Script />`). The removal logic relies on finding parentheses `&& (<Script ...)` matching the CLI injection pattern, but manual installations might omit them.
Add a test case to verify robustness against this variation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ); | ||
| } | ||
|
|
||
| const grouped = Map.groupBy(countByComponent, ([, count]) => count); |
There was a problem hiding this comment.
P1: Map.groupBy is not supported in Node.js 18 or older browsers (e.g. Chrome < 117, Safari < 17.4). Since the project supports Node 18 (engines or dependencies), this will cause runtime errors in supported environments. Use a manual grouping loop instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/react-grab/src/components/render-scan.tsx, line 83:
<comment>`Map.groupBy` is not supported in Node.js 18 or older browsers (e.g. Chrome < 117, Safari < 17.4). Since the project supports Node 18 (`engines` or `dependencies`), this will cause runtime errors in supported environments. Use a manual grouping loop instead.</comment>
<file context>
@@ -0,0 +1,465 @@
+ );
+ }
+
+ const grouped = Map.groupBy(countByComponent, ([, count]) => count);
+ const text = [...grouped.entries()]
+ .sort(([countA], [countB]) => countB - countA)
</file context>
| @@ -0,0 +1,20 @@ | |||
| import type { Component } from "solid-js"; | |||
There was a problem hiding this comment.
P2: This component deviates from the codebase's icon patterns (e.g., IconCheck, IconChevron):
- Hardcoded
whitecolors limit reusability (should usecurrentColor). - Lacks a
sizeprop for scaling.
The fix aligns it with other icons and defaults to the current 6x7 size. Ensure usages (e.g., in toolbar/index.tsx) set a text color (like text-white) if relying on the previous hardcoded value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/react-grab/src/components/icons/icon-play.tsx, line 3:
<comment>This component deviates from the codebase's icon patterns (e.g., `IconCheck`, `IconChevron`):
1. Hardcoded `white` colors limit reusability (should use `currentColor`).
2. Lacks a `size` prop for scaling.
The fix aligns it with other icons and defaults to the current 6x7 size. Ensure usages (e.g., in `toolbar/index.tsx`) set a text color (like `text-white`) if relying on the previous hardcoded value.</comment>
<file context>
@@ -0,0 +1,20 @@
+import type { Component } from "solid-js";
+
+interface IconPlayProps {
+ class?: string;
+}
</file context>
| } | ||
| > | ||
| <button class="contain-layout shrink-0 flex items-center justify-center size-3.5 rounded-full bg-black/70 hover:bg-black cursor-pointer"> | ||
| <svg |
There was a problem hiding this comment.
P2: The Play icon SVG is hardcoded here, duplicating the logic from IconPlay.
Use the existing IconPlay component (from src/components/icons/icon-play.tsx) to ensure consistency and reduce maintenance overhead. You will need to add import { IconPlay } from "./components/icons/icon-play.jsx"; to the imports.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/react-grab/src/design-system.tsx, line 2577:
<comment>The Play icon SVG is hardcoded here, duplicating the logic from `IconPlay`.
Use the existing `IconPlay` component (from `src/components/icons/icon-play.tsx`) to ensure consistency and reduce maintenance overhead. You will need to add `import { IconPlay } from "./components/icons/icon-play.jsx";` to the imports.</comment>
<file context>
@@ -2413,10 +2552,83 @@ const StateCard = (props: StateCardProps) => {
+ }
+ >
+ <button class="contain-layout shrink-0 flex items-center justify-center size-3.5 rounded-full bg-black/70 hover:bg-black cursor-pointer">
+ <svg
+ class="ml-px"
+ width="6"
</file context>
| expect(result.success).toBe(true); | ||
| expect(result.noChanges).toBeFalsy(); | ||
| expect(result.newContent).not.toContain("react-scan"); | ||
| expect(result.newContent).toContain("<head>"); |
There was a problem hiding this comment.
P2: Tests do not cover valid JSX conditionals without parentheses (e.g., && <Script />). The removal logic relies on finding parentheses && (<Script ...) matching the CLI injection pattern, but manual installations might omit them.
Add a test case to verify robustness against this variation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/test/transform.test.ts, line 919:
<comment>Tests do not cover valid JSX conditionals without parentheses (e.g., `&& <Script />`). The removal logic relies on finding parentheses `&& (<Script ...)` matching the CLI injection pattern, but manual installations might omit them.
Add a test case to verify robustness against this variation.</comment>
<file context>
@@ -887,3 +888,340 @@ describe("applyPackageJsonTransform", () => {
+ expect(result.success).toBe(true);
+ expect(result.noChanges).toBeFalsy();
+ expect(result.newContent).not.toContain("react-scan");
+ expect(result.newContent).toContain("<head>");
+ });
+
</file context>
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| ); | ||
| } | ||
|
|
||
| const grouped = Map.groupBy(countByComponent, ([, count]) => count); |
There was a problem hiding this comment.
Map.groupBy() is an ES2024 feature that causes runtime errors in unsupported browsers (Safari <18, Firefox <133, Chrome <130, Edge <130)
|
Bugbot Autofix prepared fixes for 3 of the 3 bugs found in the latest run.
|
|
Bugbot Autofix prepared fixes for 3 of the 3 bugs found in the latest run.
|
| return true; | ||
| } | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
Missing error handling for clipboard write operation
Medium Severity
The copyRecording function lacks a try-catch around navigator.clipboard.writeText(). This is inconsistent with other clipboard usage in the codebase (e.g., core/index.tsx lines 2871-2882) which properly wrap clipboard operations in try-catch blocks. If the clipboard API fails due to permissions issues or browser restrictions, this will cause an unhandled promise rejection that propagates to handleCopyRecording, potentially leaving the UI in an inconsistent state where clearLogHistory() never runs.
| stroke-linejoin="round" | ||
| > | ||
| <path d="M1 1L5 3.5L1 6V1Z" /> | ||
| </svg> |
There was a problem hiding this comment.
Duplicate play icon SVG instead of using IconPlay component
Low Severity
The play icon SVG is duplicated inline twice (lines 2577-2588 and 2603-2614) in design-system.tsx. The IconPlay component was added in this PR and is already used in toolbar/index.tsx. For consistency with IconCopy which is properly imported and used at line 2617, IconPlay should be imported and used as <IconPlay class="ml-px" /> instead of duplicating the SVG.
| stroke-linejoin="round" | ||
| > | ||
| <path d="M1 1L5 3.5L1 6V1Z" /> | ||
| </svg> |
There was a problem hiding this comment.
Two identical inline SVG play icons were duplicated in design-system.tsx instead of using the existing IconPlay component
| if (typeof navigator !== "undefined" && navigator.clipboard) { | ||
| await navigator.clipboard.writeText(text); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The copyRecording function lacks try-catch error handling around navigator.clipboard.writeText(), causing unhandled errors if clipboard operation fails
| projectInfo.packageManager, | ||
| projectInfo.projectRoot, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Package uninstall ignores detection-removal file mismatch
High Severity
Detection checks files like _app.tsx for Next.js Pages Router and src/main.tsx for Vite, but removal only targets _document.tsx or index.html respectively. When React Scan is in a detected-but-not-removed file, the package is still uninstalled unconditionally, leaving broken imports. The detectedFiles array that tracks where React Scan was found is populated but never used to prevent this inconsistency.
Additional Locations (1)
| if (typeof navigator !== "undefined" && navigator.clipboard) { | ||
| await navigator.clipboard.writeText(text); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Recording data cleared even when copy fails silently
Medium Severity
The copyRecording function silently returns without copying when navigator.clipboard is unavailable, but handleCopyRecording unconditionally calls clearLogHistory() and setHasRecordedData(false) afterward. This causes user's recorded render data to be permanently lost without being copied in environments where the clipboard API is not available (older browsers, certain security contexts).
Additional Locations (1)
|
Bugbot Autofix prepared fixes for 2 of the 2 bugs found in the latest run.
|
| ]); | ||
| label.textWidth = context.measureText(label.text).width; | ||
| labels.delete(otherKey); | ||
| } |
There was a problem hiding this comment.
Label merge loses previously merged boxes in sequential merges
Medium Severity
When merging overlapping labels, the code updates label.text using label.boxes and otherLabel.boxes, but never updates label.boxes to include the merged result. If a third label subsequently merges into the same label, it uses the original label.boxes (not the accumulated boxes), causing previously merged render information to be lost from the displayed text.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
| logger.log("No changes needed."); | ||
| logger.break(); | ||
| process.exit(0); | ||
| } |
There was a problem hiding this comment.
Migration early exit skips package uninstall when no file changes
Medium Severity
The early exit condition !hasRemovalChanges && !hasAddChanges causes the migration to exit with "No changes needed" even when react-scan package is installed in package.json but has no file references. The later package uninstall logic at lines 389-397 correctly checks reactScanInfo.isPackageInstalled && (hasRemovalChanges || reactScanInfo.detectedFiles.length === 0), which would uninstall the package when there are no file references, but this code is never reached due to the early exit.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
|
|
||
| if (reactScanInfo.isPackageInstalled) { | ||
| logger.log(` ${pc.red("−")} Uninstall ${migrationSource} package`); | ||
| } |
There was a problem hiding this comment.
Preview shows package uninstall that won't happen
Medium Severity
The preview at line 330 shows "Uninstall react-scan package" whenever reactScanInfo.isPackageInstalled is true. However, the actual uninstall at lines 393-401 only happens when isPackageInstalled && (hasRemovalChanges || detectedFiles.length === 0). When React Scan is detected in files but can't be auto-removed (detectedFiles.length > 0 and hasRemovalChanges = false), the preview misleadingly shows the uninstall action, but it won't actually be performed.
Additional Locations (1)
| const [isEnabled, setIsEnabled] = createSignal( | ||
| savedToolbarState?.enabled ?? true, | ||
| const [toolbarMode, setToolbarMode] = createSignal<ToolbarMode>( | ||
| savedToolbarState?.mode ?? "select", |
There was a problem hiding this comment.
Saved scan mode not validated against instrumentation availability
Medium Severity
When loading the saved toolbar state, the mode value is used directly without checking if it's still valid. If the saved mode is "scan" but isInstrumentationActive() returns false on page reload (e.g., React DevTools unavailable), the toolbar state becomes inconsistent: toolbarMode is "scan" so recording controls are visible and RenderScan is rendered, but the ModeSelector shows the knob at the "off" position because it only displays 2 options when scan mode is unavailable.
|
|
||
| if (!hasRemovalChanges && !hasAddChanges) { | ||
| exitWithMessage("No changes needed."); | ||
| } |
There was a problem hiding this comment.
Migration succeeds silently when detected files cannot be cleaned
Medium Severity
When React Scan is detected in files (e.g., _app.tsx) that the removal logic doesn't target (which only handles _document.tsx for Pages Router), the migration proceeds without warning. The hasReactGrab branch has validation at lines 209-222 that warns about detectedFiles that couldn't be cleaned, but the main migration branch lacks this check. Users see "Success! Migration complete." while React Scan code remains in their project.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/cli/src/commands/migrate.ts">
<violation number="1" location="packages/cli/src/commands/migrate.ts:306">
P2: The uninstall logic is unsafe when React Scan is used in multiple files.
If `detectReactScan` finds usages in multiple files (e.g., `layout.tsx` and `index.html`), but `previewReactScanRemoval` only targets one file (e.g., `layout.tsx`), `hasRemovalChanges` will be true. This satisfies the condition `(hasRemovalChanges || ...)` and causes the package to be uninstalled, breaking the application because the reference in `index.html` remains.
You should verify that *all* detected files are covered by the removal transform before uninstalling.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
|
|
||
| if (!hasRemovalChanges && !hasAddChanges) { | ||
| exitWithMessage("No changes needed."); | ||
| } |
There was a problem hiding this comment.
Migration reports success when React Grab code cannot be added
Medium Severity
When previewTransform returns success: false (e.g., layout file not found), the migration doesn't check for this failure. If removal succeeds but addition fails, the migration removes React Scan, installs the react-grab package, but does NOT add React Grab code to the project. The migration still reports "Success!" without showing the error message from addResult.message. This leaves users with a broken setup where neither tool works.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
- Updated detection logic for React Scan in various file types, including package.json and layout files. - Improved the removal process for React Scan scripts in Next.js, Vite, and Webpack configurations. - Added comprehensive tests to ensure accurate detection and removal functionality across different scenarios. - Refactored related utility functions for better maintainability and performance.
…iled copy - Fix package uninstall when React Scan detected in non-targeted files - Prevent recording data loss when clipboard API unavailable - Only uninstall react-scan package when code removal succeeds or no code detected - Make copyRecording return boolean to indicate success/failure
When merging overlapping labels, update label.boxes to include the merged boxes so that subsequent merges don't lose previously merged render information.
…changes When react-scan is installed in package.json but has no file references, the migration should still proceed to uninstall the package rather than exiting early with 'No changes needed'. This fix adds a check for needsPackageUninstall to the early exit condition.
…in migrate command
* feat: add comment mode to toolbar - Add comment button to toolbar that enters prompt mode on next element click - Add pendingCommentMode state to store - Add isCommentMode and onComment props to toolbar and renderer - Extract common toolbar button handlers (stopEventPropagation, createFreezeHandlers) - Replace nested ternaries with getToolbarIconColor helper function - Fix overflow class inconsistency in toolbar-content.tsx - Remove unused formatShortcut import * chore: update version in manifest.json to 0.0.7 * fix: clear pendingCommentMode on drag selection to prevent unexpected prompt mode * feat: enhance comment mode functionality - Refactor comment mode handling in the toolbar and renderer - Update state management for pendingCommentMode and isCommentMode - Improve element interaction logic for entering comment mode - Ensure proper activation and deactivation of comment mode based on user actions * feat: enhance toolbar functionality with new props - Add isToolbarCommentMode and isToolbarCollapsed props to the design system - Update toolbar content to utilize new props for improved state management - Refactor button rendering logic in toolbar-content.tsx to accommodate comment mode and active states * fix: comment mode drag selection and toolbar button styling - Remove premature setPendingCommentMode(false) in handleDragSelection to allow comment mode for drag selections - Add isCommentMode prop to ToolbarContentProps and apply conditional styling to defaultCommentButton * Fix comment icon state inconsistency and extract duplicate icon color helper - Fix comment icon toggle behavior to check isCommentMode() instead of just pendingCommentMode, ensuring visual state matches click behavior - Extract getToolbarIconColor to shared utility to eliminate code duplication between toolbar files * fix: rename confusing isActive parameter to isDimmed in getToolbarIconColor --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
- Updated detection logic for React Scan in various file types, including package.json and layout files. - Improved the removal process for React Scan scripts in Next.js, Vite, and Webpack configurations. - Added comprehensive tests to ensure accurate detection and removal functionality across different scenarios. - Refactored related utility functions for better maintainability and performance.
…iled copy - Fix package uninstall when React Scan detected in non-targeted files - Prevent recording data loss when clipboard API unavailable - Only uninstall react-scan package when code removal succeeds or no code detected - Make copyRecording return boolean to indicate success/failure
- Improved migration command to handle non-interactive mode more gracefully, including confirmation prompts and error handling. - Refactored toolbar components to incorporate new selection modes and improve state management. - Updated utility functions for better icon color handling based on selection mode. - Enhanced type definitions for better clarity and maintainability across the codebase.
| [migrationSource], | ||
| projectInfo.packageManager, | ||
| projectInfo.projectRoot, | ||
| ); |
There was a problem hiding this comment.
Package uninstalled while react-scan imports remain in uncleaned files
Medium Severity
The detection in detectReactScan checks multiple files (e.g., both _document.tsx and _app.tsx for Pages Router), but findReactScanFile only returns one file per framework. When react-scan exists in multiple files, the removal only cleans one, yet the uninstall condition at line 361 (hasRemovalChanges || reactScanInfo.detectedFiles.length === 0) evaluates to true if ANY removal succeeded. This causes the package to be uninstalled while other files still have react-scan imports, breaking those files at runtime.
Additional Locations (1)
Previously, react-scan package was uninstalled after cleaning ANY file with react-scan imports, even when other files still contained react-scan code. This caused runtime errors when react-scan existed in multiple files (e.g., both _document.tsx and _app.tsx in Next.js Pages Router). Now checks that all detected files have been cleaned before uninstalling the package.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
packages/cli/src/commands/migrate.ts
Outdated
| addResult, | ||
| `Adding React Grab to ${addResult.filePath}.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Package uninstall happens before file transforms, risking inconsistent state
Medium Severity
In the main migration path, uninstallPackagesWithFeedback and installPackagesWithFeedback are called before applyTransformWithFeedback runs. If the file transforms fail (e.g., due to permissions), the project is left with react-scan uninstalled and react-grab installed, but the files still contain react-scan code and lack react-grab code. This contradicts the PR description stating "only uninstall react-scan when removals succeed." The other code path (lines 260-275 for when React Grab is already installed) correctly applies the transform first, then uninstalls the package.
|
|
||
| if (reactScanInfo.isPackageInstalled) { | ||
| logger.log(` ${pc.red("−")} Uninstall ${migrationSource} package`); | ||
| } |
There was a problem hiding this comment.
Preview shows package uninstall that won't actually happen
Medium Severity
The preview message at lines 315-317 shows "Uninstall react-scan package" whenever reactScanInfo.isPackageInstalled is true. However, the actual uninstall at lines 370-376 has an additional condition: otherDetectedFiles.length === 0. If react-scan code exists in files that weren't cleaned by the removal transform (e.g., detected in src/main.tsx but removal targeted index.html), the preview will claim the package will be uninstalled, but it won't be. The preview condition needs to match the execution condition.
Additional Locations (1)
…view condition - Apply file transforms before uninstalling/installing packages to avoid inconsistent state if transforms fail - Fix preview message to accurately show when react-scan package will be uninstalled by adding otherDetectedFiles.length check
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/cli/src/commands/migrate.ts">
<violation number="1" location="packages/cli/src/commands/migrate.ts:269">
P2: The execution condition for package uninstall includes `otherDetectedFiles.length === 0`, but the preview logic (lines 314-317) may not include this same condition. This can cause the preview to incorrectly show "Uninstall react-scan package" in cases where the uninstall won't actually happen due to remaining detected files. The preview condition should match this execution condition to avoid misleading users.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| (file) => file !== removalResult.filePath, | ||
| ); | ||
|
|
||
| if (reactScanInfo.isPackageInstalled && otherDetectedFiles.length === 0) { |
There was a problem hiding this comment.
P2: The execution condition for package uninstall includes otherDetectedFiles.length === 0, but the preview logic (lines 314-317) may not include this same condition. This can cause the preview to incorrectly show "Uninstall react-scan package" in cases where the uninstall won't actually happen due to remaining detected files. The preview condition should match this execution condition to avoid misleading users.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/commands/migrate.ts, line 269:
<comment>The execution condition for package uninstall includes `otherDetectedFiles.length === 0`, but the preview logic (lines 314-317) may not include this same condition. This can cause the preview to incorrectly show "Uninstall react-scan package" in cases where the uninstall won't actually happen due to remaining detected files. The preview condition should match this execution condition to avoid misleading users.</comment>
<file context>
@@ -262,7 +262,11 @@ export const migrate = new Command()
+ (file) => file !== removalResult.filePath,
+ );
+
+ if (reactScanInfo.isPackageInstalled && otherDetectedFiles.length === 0) {
uninstallPackagesWithFeedback(
[migrationSource],
</file context>
|
Bugbot Autofix prepared fixes for 2 of the 2 bugs found in the latest run.
|
Note
Medium Risk
Touches CLI automation that rewrites project files and uninstalls/installs packages; regex-based detection/removal could miss variants or over-remove code in edge cases despite added tests.
Overview
Adds a new
grab migrateCLI command to migrate from React Scan by detectingreact-scanusage (inpackage.jsonand common entry/config files), previewing diffs, applying automated removals, and installingreact-grab(optionally uninstallingreact-scanonly when safe).Extends the CLI utilities with React Scan detection patterns + file targeting and a
previewReactScanRemovaltransform for Next.js/Vite/Webpack/TanStack setups, backed by comprehensive new unit tests; also adds a Gym dashboardPerformanceTestpage component to intentionally create slow renders/effects for performance tooling validation.Written by Cursor Bugbot for commit bf5b0c9. This will update automatically on new commits. Configure here.
Summary by cubic
Adds a new Scan mode to visualize and record component renders, plus a CLI migrate command to detect and remove react-scan across common setups. Adds a comment mode, makes drag selection copy by default, improves viewport handling, and keeps uninstalls and logging safe.
New Features
Migration
Written for commit bf5b0c9. Summary will update on new commits.