Conversation
…l and during messaging. Add merge view and minimap extensions to CodeMirror for enhanced code editing capabilities. Increase Express JSON and URL-encoded payload limits from default (100kb) to 50mb to support larger file operations and git diffs.
Calculate token rate once per timing session instead of recalculating on every interval tick. This prevents the simulated token count from jumping erratically due to random value changes. Also remove noisy console warning in websocket utility that was cluttering development logs without providing actionable information.
…epth Wrap markSessionAsActive, markSessionAsInactive, markSessionAsProcessing, markSessionAsNotProcessing, and replaceTemporarySession functions in useCallback hooks to prevent unnecessary re-renders and stabilize function references across component lifecycle. This optimization ensures child components receiving these callbacks won't re-render unnecessarily when AppContent re-renders.
WalkthroughThis PR enhances the application's diff viewing capabilities by introducing a new backend API endpoint for retrieving files with diff context, strengthening git repository validation, and refactoring frontend components to propagate diff information through the file-opening workflow. The code editor now renders diffs using merge-view with minimap support. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant GitPanel
participant API as Backend API
participant CodeEditor
participant DiffView as Merge View + Minimap
User->>GitPanel: Click file path
GitPanel->>API: GET /file-with-diff?filePath
API->>API: Detect file status (untracked/deleted/tracked)
API->>API: Compute diff via git or /dev/null
API-->>GitPanel: {currentContent, oldContent, isDeleted, isUntracked}
GitPanel->>CodeEditor: onFileOpen(filePath, diffInfo)
CodeEditor->>DiffView: Initialize unifiedMergeView
DiffView->>DiffView: Render old_string vs new_string
DiffView->>DiffView: Compute and highlight chunks
DiffView->>DiffView: Render minimap gutter
DiffView-->>User: Display diff with navigation & minimap
User->>DiffView: Navigate chunks (Prev/Next)
DiffView->>DiffView: scrollToFirstChunkExtension activates
DiffView-->>User: Scroll to chunk position
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/index.js (1)
412-419: Block symlink traversal before reading/writing files.The new
path.resolve(...).startsWith(normalizedRoot)guard can be bypassed via symlinks. If a symlink inside the project points outside (e.g.projectRoot/link -> /etc), a request forlink/passwdresolves to…/projectRoot/link/passwd, so the string check passes even thoughfs.readFile/writeFileoperates on/etc/passwd. Similar prefix logic has been exploited in other Node servers (see CVE-2025-58751 for sirv) because it only compares strings rather than the canonical target.(miggo.io)Please resolve the absolute real path before validating, e.g.:
const resolved = await fs.promises.realpath( path.isAbsolute(filePath) ? filePath : path.join(projectRoot, filePath) ); const normalizedRoot = (await fs.promises.realpath(projectRoot)) + path.sep; if (!resolved.startsWith(normalizedRoot)) { … deny … }and apply the same fix to both the GET and PUT endpoints.
Also applies to: 511-518
src/components/ChatInterface.jsx (1)
467-526: Refactor: Extract duplicate async file-fetching logic.Lines 468-492 and 503-525 contain nearly identical async file-fetching logic. This duplication makes maintenance harder and increases the risk of inconsistencies.
Suggestion: Extract a shared helper function:
const fetchFileForDiff = async (filePath, isNewFile = false) => { if (!onFileOpen || !selectedProject?.name) return; try { const response = await api.readFile(selectedProject.name, filePath); const data = await response.json(); const newContent = (response.ok && !data.error) ? data.content || '' : input.content || ''; onFileOpen(filePath, { old_string: isNewFile ? '' : undefined, // Let caller specify old_string for edits new_string: newContent }); } catch (error) { console.error('Error preparing diff:', error); if (isNewFile) { onFileOpen(filePath, { old_string: '', new_string: input.content || '' }); } else { onFileOpen(filePath); } } };Then use:
onClick={() => fetchFileForDiff(input.file_path, true)}This also applies to the duplicated logic in lines 924-943 and 962-981.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.json(2 hunks)server/index.js(3 hunks)server/routes/git.js(2 hunks)src/App.jsx(3 hunks)src/components/ChatInterface.jsx(10 hunks)src/components/ClaudeStatus.jsx(1 hunks)src/components/CodeEditor.jsx(11 hunks)src/components/GitPanel.jsx(5 hunks)src/components/MainContent.jsx(1 hunks)src/utils/websocket.js(0 hunks)
💤 Files with no reviewable changes (1)
- src/utils/websocket.js
🧰 Additional context used
🧬 Code graph analysis (5)
server/index.js (4)
server/routes/git.js (1)
filePath(476-476)server/utils/commandParser.js (1)
filePath(126-126)server/projects.js (1)
filePath(103-103)server/routes/taskmaster.js (6)
filePath(141-141)filePath(727-727)filePath(801-801)filePath(869-869)filePath(934-934)filePath(1897-1897)
src/components/CodeEditor.jsx (2)
src/contexts/ThemeContext.jsx (1)
isDarkMode(15-28)src/utils/api.js (2)
api(23-141)api(23-141)
src/components/MainContent.jsx (2)
src/App.jsx (2)
selectedProject(49-49)isMobile(52-52)src/components/GitPanel.jsx (1)
handleFileOpen(423-451)
src/components/ChatInterface.jsx (2)
src/utils/api.js (4)
api(23-141)api(23-141)authenticatedFetch(2-20)authenticatedFetch(2-20)src/App.jsx (1)
selectedProject(49-49)
src/components/GitPanel.jsx (2)
src/components/MainContent.jsx (1)
handleFileOpen(117-126)src/utils/api.js (2)
authenticatedFetch(2-20)authenticatedFetch(2-20)
🔇 Additional comments (8)
package.json (1)
49-82: Summary inconsistency: dependencies show as present in final state.The AI-generated summary claims the four CodeMirror/xterm packages were "added then removed" with only
wspersisting, but the code clearly shows@codemirror/merge,@replit/codemirror-minimap,@xterm/addon-fit, and@xterm/xtermall present in lines 49, 51, 55, and 57.The added dependencies are well-aligned with the PR objectives (diff viewing with merge-view and minimap support). The versions use standard caret ranges and appear reasonable.
src/components/ClaudeStatus.jsx (1)
10-29: LGTM! Improved token progression behavior.The refactor to calculate
tokenRateonce per loading session (rather than recalculating every second) results in smoother, more predictable token count progression. This improves the user experience and slightly reduces computational overhead.src/App.jsx (4)
21-21: Good addition to support performance optimizations.The
useCallbackimport is necessary for the function memoization applied to session lifecycle helpers below.
185-188: LGTM: Cleaner conditional logic.Removing the else-branch and verbose console logs simplifies the code while preserving the correct behavior—messages are only reloaded when the session is not active.
481-516: Excellent performance optimization with useCallback.Wrapping the session lifecycle helpers (
markSessionAsActive,markSessionAsInactive,markSessionAsProcessing,markSessionAsNotProcessing) inuseCallbackwith empty dependency arrays is correct and beneficial:
- All functions use the setState function updater form, so they don't require external dependencies
- Stabilizes function references passed to
MainContent(lines 717-720)- Prevents unnecessary re-renders when these callbacks haven't changed
521-535: LGTM: Enhanced robustness with useCallback.The
replaceTemporarySessionfunction now explicitly preserves all non-temporary session IDs before adding the real session ID. This is more robust than the previous approach and prevents accidentally clearing other active sessions during the temporary-to-real session ID transition.The empty dependency array is correct since the function uses the setState updater form.
src/components/ChatInterface.jsx (2)
173-173: LGTM: Clean prop propagation for selectedProject.The addition of
selectedProjecttoMessageComponentand its proper threading through the component hierarchy enables the async file-fetching logic for diff views. The implementation is clean and follows React best practices.Also applies to: 4092-4092
923-982: Verify the new/api/git/file-with-diffendpoint exists.The endpoint is confirmed to exist at
server/routes/git.js:197, so this git-based diffing approach is properly implemented. This is a solid improvement over the naive string replacement used in the Edit tool.Code duplication between lines 924-943 and 962-981 — Extract this identical logic into a helper function or utility to follow DRY principles. Better yet, replace the Edit tool button logic (lines 318-386) to use this same git-based endpoint instead of regex-based string replacement, ensuring consistent behavior across all file buttons.
| <button | ||
| onClick={async (e) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| onFileOpen && onFileOpen(input.file_path, { | ||
| old_string: input.old_string, | ||
| new_string: input.new_string | ||
| }); | ||
| if (!onFileOpen) return; | ||
|
|
||
| try { | ||
| // Fetch the current file (after the edit) | ||
| const response = await api.readFile(selectedProject?.name, input.file_path); | ||
| const data = await response.json(); | ||
|
|
||
| if (!response.ok || data.error) { | ||
| console.error('Failed to fetch file:', data.error); | ||
| onFileOpen(input.file_path); | ||
| return; | ||
| } | ||
|
|
||
| const currentContent = data.content || ''; | ||
|
|
||
| // Reverse apply the edit: replace new_string back to old_string to get the file BEFORE the edit | ||
| const oldContent = currentContent.replace(input.new_string, input.old_string); | ||
|
|
||
| // Pass the full file before and after the edit | ||
| onFileOpen(input.file_path, { | ||
| old_string: oldContent, | ||
| new_string: currentContent | ||
| }); | ||
| } catch (error) { | ||
| console.error('Error preparing diff:', error); | ||
| onFileOpen(input.file_path); | ||
| } | ||
| }} |
There was a problem hiding this comment.
Critical: Naive string replacement breaks diff calculation.
The reverse-apply logic on lines 338 and 375 has several critical flaws:
String.replace()only replaces the first occurrence - ifinput.new_stringappears multiple times in the file, only the first match is reversed, producing an incorrect diff.- No validation - if
input.new_stringisn't found (e.g., file was modified after the edit),replace()silently returns the original string unchanged, and the diff will show the entire file as "old" content. - Race condition - the file might have changed between tool execution and button click, making the diff stale or incorrect.
Fix: Use a more robust approach:
- // Reverse apply the edit: replace new_string back to old_string to get the file BEFORE the edit
- const oldContent = currentContent.replace(input.new_string, input.old_string);
+ // Reverse apply the edit: replace new_string back to old_string
+ const newStringIndex = currentContent.indexOf(input.new_string);
+ if (newStringIndex === -1) {
+ console.error('Could not reverse edit: new_string not found in current file');
+ onFileOpen(input.file_path);
+ return;
+ }
+ const oldContent =
+ currentContent.slice(0, newStringIndex) +
+ input.old_string +
+ currentContent.slice(newStringIndex + input.new_string.length);Alternatively, call the new /api/git/file-with-diff endpoint (used in lines 929, 967) here as well, which appears to provide proper git-based diffing.
Also applies to: 358-386
🤖 Prompt for AI Agents
In src/components/ChatInterface.jsx around lines 318-349 and also where similar
logic appears at 358-386, the current reverse-apply logic uses String.replace
which only replaces the first occurrence, has no validation if the new string
isn't found, and is susceptible to race conditions; replace this whole block
with a call to the existing /api/git/file-with-diff endpoint (same one used at
lines ~929/967) to retrieve a git-accurate {before, after} payload and pass
those into onFileOpen; if the endpoint call fails, fall back to fetching the
current file and compute a validated diff (find the exact location of
input.new_string using robust index-based search or a diff library, verify it
was found before substituting, and log and fallback to onFileOpen(path) when
validation fails) so you never silently return an incorrect full-file "old"
content.
Feature/edit diff
1 participant
Summary by CodeRabbit
New Features
Bug Fixes