feat: Phase 1-2 core IDE shell with editor and terminal#1
Merged
Conversation
Set up the full development skeleton: electron-vite build pipeline, pnpm workspace monorepo (main/renderer/shared), BaseWindow with WebContentsView, typed IPC channels, Atom One Dark/Light theme tokens, frameless window with macOS traffic lights, ESLint 9 + Prettier + TypeScript strict mode. Add Vitest unit tests and Playwright e2e tests for PR CI gates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Build the IDE chrome: workspace ribbon (draggable, with agent status dots and theme toggle), resizable sidebar, status bar, and a 3-pane Dockview layout (Editor, Terminal, Agent). Theme persists across restarts via electron-store. Completes Phase 1.3 + 1.4. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tighten the entire UI for professional IDE feel: 11px base font, compact ribbon/status bar, custom scrollbars, SVG icons, smooth transitions. Add Electron app menu for native Cmd+/-/0 zoom and Cmd+C/V/X clipboard. Fix fullscreen ribbon padding and oversized Dockview close buttons. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sistence Implement browsable read-only file tree in the fixed sidebar (phase 2.1a). Adds native open-folder dialog, lazy directory reading via IPC, sidebar width persistence, and centralizes WindowApi type in shared package. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…hlighting Integrate CodeMirror 6 as the editor component with support for JS/TS, Python, Markdown, JSON, CSS, and HTML syntax highlighting. Files open in Dockview tabs from file tree clicks, with EditorState caching to preserve cursor/scroll across tab switches. Adds readFile IPC with 10MB limit and binary rejection, theme hot-swap via Compartment, and real line/col/language reporting in the status bar. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implement writeFile IPC for Cmd+S save, dirty state tracking with • tab indicator and confirm-before-close for unsaved tabs via DockviewDefaultTab with closeActionOverride. Add indent guides via @replit/codemirror-indentation-markers and word wrap toggle (Cmd+Alt+W) via Compartment. Update build plan to reflect 2.2b completion. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wire up xterm.js 6 + node-pty for a real embedded terminal. Implements UUID-multiplexed PTY IPC (create/data-in/data-out/resize/kill/exit), FitAddon + WebLinksAddon with appActions dispatch, ResizeObserver with debounced ptyResize, theme hot-swap, Cmd+Shift+T for new terminal tabs, and destroyed-flag async safety. Externalizes node-pty as a native addon in the Vite config. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a new Electron + Vite monorepo IDE: root tooling/config, three workspace packages (main, renderer, shared), an Electron main process with IPC and PTY management, a preload bridge exposing a typed Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (React UI)
participant Preload as Preload (contextBridge)
participant Main as Main Process (Electron)
participant PTY as PTY (node-pty)
participant FS as Filesystem
Renderer->>Preload: call window.api.ptyCreate(opts)
Preload->>Main: ipcRenderer.invoke(IpcChannels.PTY_CREATE, opts)
Main->>PTY: spawn PTY (shell, cwd)
PTY-->>Main: onData / onExit events
Main->>Preload: ipcMain.send(IpcChannels.PTY_DATA_OUT, id, data)
Preload->>Renderer: invokes subscribed callback (onPtyData)
Renderer->>Preload: window.api.ptyWrite(id, data)
Preload->>Main: ipcRenderer.send(IpcChannels.PTY_DATA_IN, id, data)
Main->>PTY: write(data)
Renderer->>Preload: window.api.readFile(path)
Preload->>Main: ipcRenderer.invoke(IpcChannels.FS_READ_FILE, path)
Main->>FS: read file (validate size/utf8)
FS-->>Main: file content / error
Main-->>Preload: return content or error
Preload-->>Renderer: Promise resolves/rejects
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (17)
playwright.config.ts (1)
6-9: Trace won't trigger with zero retries.
trace: 'on-first-retry'captures traces only when a test is retried, butretries: 0means no retries occur. If you want traces on failure without retries, considertrace: 'retain-on-failure'.💡 Suggested change
retries: 0, use: { - trace: 'on-first-retry', + trace: 'retain-on-failure', },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright.config.ts` around lines 6 - 9, The Playwright config sets retries: 0 so trace: 'on-first-retry' will never run; change the trace strategy to capture traces on failure by updating the trace setting under use (the use.trace option) to 'retain-on-failure' (or another appropriate mode) so traces are collected without enabling retries; locate the retries property and the use.trace entry in the Playwright config (playwright.config.ts) and replace the trace value accordingly.packages/shared/src/index.test.ts (1)
17-23: Add a uniqueness assertion for IPC channel values.Current checks confirm non-empty strings, but not uniqueness. A duplicate value would silently alias two channels.
✅ Minimal assertion to add
it('has string literal values (no accidental undefined)', () => { const values = Object.values(IpcChannels) for (const v of values) { expect(typeof v).toBe('string') expect(v.length).toBeGreaterThan(0) } + expect(new Set(values).size).toBe(values.length) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/index.test.ts` around lines 17 - 23, The test "has string literal values (no accidental undefined)" validates non-empty strings but misses duplicates; update that test (the one iterating over Object.values(IpcChannels)) to assert uniqueness by creating a Set from the values and expecting its size to equal the original values.length (or otherwise fail if a duplicate exists), so any duplicate IPC channel string in IpcChannels is caught.packages/shared/package.json (1)
5-6: Point package entrypoints to build output, not TS source.Line [5] and Line [6] currently expose
.tsfiles as package entrypoints. That can break Node/Electron runtime resolution if@aide/sharedis externalized instead of bundled. PreferdistJS + declaration outputs.♻️ Suggested change
{ "name": "@aide/shared", "version": "0.1.0", "private": true, - "main": "./src/index.ts", - "types": "./src/index.ts" + "main": "./dist/index.js", + "types": "./dist/index.d.ts" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/package.json` around lines 5 - 6, The package.json currently exposes TypeScript sources via the "main" and "types" fields; update these fields to point to the compiled outputs (e.g., set "main" to the built JS entry like ./dist/index.js and "types" to the declaration file like ./dist/index.d.ts) and ensure the build step produces those files (emitDeclarationOnly/tsconfig "declaration": true or your bundler emits .d.ts) so consumers import runtime JS and type declarations instead of .ts sources.tests/e2e/launch.test.ts (1)
8-36: Consider extracting common test setup to reduce duplication.Both tests share identical Electron launch configuration. Extracting this to a
beforeEach/afterEachpattern or a helper would reduce repetition and ensure consistent teardown.♻️ Proposed refactor using test hooks
+import type { ElectronApplication, Page } from '@playwright/test' + const rootDir = resolve(__dirname, '../..') test.describe('App launch', () => { + let app: ElectronApplication + let window: Page + + test.beforeEach(async () => { + app = await electron.launch({ + args: [resolve(rootDir, 'packages/main/dist/index.js')], + cwd: rootDir, + }) + window = await app.firstWindow() + await window.waitForLoadState('domcontentloaded') + }) + + test.afterEach(async () => { + await app.close() + }) + test('opens a window with the correct title', async () => { - const app = await electron.launch({ - args: [resolve(rootDir, 'packages/main/dist/index.js')], - cwd: rootDir, - }) - - const window = await app.firstWindow() - await window.waitForLoadState('domcontentloaded') - const title = await window.title() expect(title).toBe('aIDE') - - await app.close() }) test('renders the app shell content', async () => { - const app = await electron.launch({ - args: [resolve(rootDir, 'packages/main/dist/index.js')], - cwd: rootDir, - }) - - const window = await app.firstWindow() - await window.waitForLoadState('domcontentloaded') - await expect(window.locator('h1')).toHaveText('aIDE') await expect(window.locator('.app-shell')).toBeVisible() - - await app.close() }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/launch.test.ts` around lines 8 - 36, Extract the duplicated electron launch/teardown into shared test setup: create a beforeEach that launches Electron with the same args/cwd (the current electron.launch call), assigns the returned object to a shared app variable and awaits app.firstWindow() assigned to a shared window variable, and create an afterEach that calls app.close(); then update both tests ('opens a window with the correct title' and 'renders the app shell content') to use the shared app/window instead of repeating the electron.launch/firstWindow/waitForLoadState/close sequence (or alternatively factor the launch logic into a helper function used by beforeEach).packages/renderer/src/components/StatusBar.tsx (1)
20-23: Hardcoded branch name is a placeholder.The branch name
mainis hardcoded. This appears intentional for the Phase 1-2 scaffold, but consider adding a TODO comment or tracking this for future git integration.📝 Suggested TODO comment
<span className="status-bar__item status-bar__item--branch"> <GitBranchIcon /> - main + main {/* TODO: Replace with dynamic git branch from IPC */} </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/components/StatusBar.tsx` around lines 20 - 23, The StatusBar component currently renders a hardcoded branch name "main" next to GitBranchIcon; add a clear TODO comment above the span (or inside the StatusBar component) indicating this is a placeholder and must be replaced with dynamic git integration later (e.g., replace with a branchName prop or a git service lookup); reference the span with class "status-bar__item--branch" and the GitBranchIcon so reviewers can find the exact location and include a short tracking note (ticket/issue) or "TODO" tag for future work.tests/unit/app.test.tsx (1)
34-39: Consider resetting mocks between tests.As the test suite grows, accumulated mock call history could cause test pollution. Adding
vi.clearAllMocks()inbeforeEachensures each test starts with a clean slate.🧹 Proposed fix
beforeEach(() => { + vi.clearAllMocks() Object.defineProperty(window, 'api', { value: mockApi, writable: true, }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/app.test.tsx` around lines 34 - 39, The test setup's beforeEach currently defines window.api to mockApi but doesn't reset mock history; add a call to vi.clearAllMocks() at the start of the beforeEach so mock call/instance history is reset before each test, keeping window.api assignment (Object.defineProperty on window, 'api') intact and ensuring mockApi starts fresh for every test.packages/renderer/src/components/panes/EditorTab.tsx (1)
8-18: LGTM!The close interception logic correctly guards against accidental data loss. The dirty check and confirmation flow are properly implemented.
For future polish, consider replacing
window.confirmwith a custom modal dialog that matches the app's theme and provides a more integrated UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/components/panes/EditorTab.tsx` around lines 8 - 18, Replace the synchronous browser confirm with the app's themed confirmation modal: modify EditorTab's handleClose to be async and await a promise-based confirmation dialog instead of using window.confirm, invoke the app modal (e.g. your ConfirmModal or useModal hook) to show "Discard unsaved changes?" and resolve true/false, and then call props.api.close() only if the modal resolved true; update the closeActionOverride prop on DockviewDefaultTab to pass the new async handleClose and ensure the modal component returns a boolean so isDirty(filePath) + handleClose logic remains intact.packages/renderer/src/components/FileTree/FileTreeItem.tsx (1)
3-9: Consider extracting sharedFileTreeNodeinterface.This interface is duplicated in
FileTree.tsx. Extracting it to a shared location (e.g.,types.tsin this directory) would improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/components/FileTree/FileTreeItem.tsx` around lines 3 - 9, Extract the duplicated FileTreeNode interface into a single shared declaration file (e.g., create a new types.ts in the same directory), export the interface from that file, then update FileTreeItem.tsx and FileTree.tsx to import FileTreeNode instead of declaring it locally; remove the local duplicate interface declarations so both components use the shared FileTreeNode type to ensure a single source of truth.packages/renderer/src/components/FileTree/FileTree.tsx (2)
117-118: Non-null assertion flagged by ESLint.The assertion is logically safe here since we iterate over
entriesand add tonext, but consider using optional chaining or a guard for consistency with the linting rules.♻️ Proposed fix
- const current = next.get(path)! - next.set(path, { ...current, isExpanded: true, isLoaded: true, children: childPaths }) + const current = next.get(path) + if (current) { + next.set(path, { ...current, isExpanded: true, isLoaded: true, children: childPaths }) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/components/FileTree/FileTree.tsx` around lines 117 - 118, Replace the non-null assertion on the Map lookup in FileTree.tsx by guarding the result of next.get(path) instead of using "!"—retrieve current via const current = next.get(path) and if current is undefined skip or continue the loop (or handle a sensible default) before calling next.set(path, { ...current, isExpanded: true, isLoaded: true, children: childPaths }); this removes the ESLint non-null assertion while preserving the existing logic around entries/next.
128-129: Non-null assertion flagged by ESLint.While safe due to the
stack.length > 0check, you can satisfy the linter by using a guard:♻️ Proposed fix
while (stack.length > 0) { - const p = stack.pop()! + const p = stack.pop() + if (!p) continue const node = nodes.get(p)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/components/FileTree/FileTree.tsx` around lines 128 - 129, The non-null assertion on stack.pop() in the FileTree component's loop should be removed to satisfy ESLint: replace the expression "const p = stack.pop()!" with a safe guarded pop (e.g., assign without "!" and early-continue or if-check) so that when stack.pop() returns undefined you handle it explicitly before using p; update the loop body where p is used (the variable named p from stack.pop()) to rely on the guard rather than a non-null assertion.packages/renderer/src/hooks/useTheme.ts (1)
15-29: Consider handlinggetTheme()rejection.If the IPC call fails, the promise rejection is unhandled. A
.catch()would prevent potential unhandled rejection warnings and allow fallback to the default theme.♻️ Proposed fix
// Load persisted theme from main process - window.api.getTheme().then((t) => { - setTheme(t) - document.documentElement.setAttribute('data-theme', t) - }) + window.api.getTheme() + .then((t) => { + setTheme(t) + document.documentElement.setAttribute('data-theme', t) + }) + .catch(() => { + // Keep default theme on error + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/hooks/useTheme.ts` around lines 15 - 29, The useEffect currently calls window.api.getTheme() without handling promise rejection; wrap or chain a .catch on window.api.getTheme() to handle errors (log the error and fall back to a default theme) and ensure you still call setTheme(...) and document.documentElement.setAttribute('data-theme', ...) with the fallback value; also ensure window.api.onThemeChanged usage remains unchanged for updates. Use the existing identifiers (useEffect, window.api.getTheme, window.api.onThemeChanged, setTheme, document.documentElement.setAttribute) to locate where to add the .catch and fallback logic.packages/renderer/src/components/AppShell.tsx (1)
16-22: Stale closure risk withregisterAppActions.The
onFileOpenfunction is captured in the closure at mount time. IfonFileOpenwere to change (e.g., if it gained dependencies), the registered action would reference a stale version. The current implementation works becauseonFileOpenhas an empty dependency array, but this coupling is fragile.Consider passing the ref-based approach or re-registering when
onFileOpenchanges:♻️ Proposed fix using ref indirection
export function AppShell() { const dockviewApiRef = useRef<DockviewApi | null>(null) + const onFileOpenRef = useRef<(filePath: string) => void>(() => {}) const onApiReady = useCallback((api: DockviewApi) => { dockviewApiRef.current = api }, []) + // Keep ref updated + useEffect(() => { + onFileOpenRef.current = onFileOpen + }, [onFileOpen]) // Register app-wide action dispatch layer useEffect(() => { registerAppActions({ - openFile: (filePath: string) => onFileOpen(filePath), + openFile: (filePath: string) => onFileOpenRef.current(filePath), openUrl: (url: string) => window.open(url), }) - }, []) // eslint-disable-line react-hooks/exhaustive-deps + }, [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/components/AppShell.tsx` around lines 16 - 22, The registered handler for registerAppActions captures onFileOpen at mount, risking a stale closure; change to one of two fixes: either store the latest onFileOpen in a ref (e.g., create a ref currentOnFileOpen and update it whenever onFileOpen changes, then register a stable callback that calls currentOnFileOpen.current), or make the useEffect re-run when onFileOpen changes (add onFileOpen to the dependency array) so registerAppActions is re-registered with the new function; update the code around useEffect, registerAppActions, and onFileOpen accordingly.packages/main/src/index.ts (1)
191-211: Consider optimizing binary file detection for large files.The current approach reads the entire file (up to 10MB) as UTF-8 before checking for binary content. For large binary files, this is wasteful since you only need the first 8KB to detect null bytes.
Proposed optimization
ipcMain.handle(IpcChannels.FS_READ_FILE, async (_event, filePath: string): Promise<{ content: string } | { error: string }> => { try { const info = await stat(filePath) if (!info.isFile()) return { error: 'Not a file' } if (info.size > MAX_FILE_SIZE) return { error: `File too large (${(info.size / 1024 / 1024).toFixed(1)} MB). Maximum is 10 MB.` } + // Quick binary check on first 8KB before reading the full file + const { createReadStream } = await import('fs') + const sampleBuffer = await new Promise<Buffer>((resolve, reject) => { + const chunks: Buffer[] = [] + const stream = createReadStream(filePath, { start: 0, end: 8191 }) + stream.on('data', (chunk: Buffer) => chunks.push(chunk)) + stream.on('end', () => resolve(Buffer.concat(chunks))) + stream.on('error', reject) + }) + if (sampleBuffer.includes(0)) return { error: 'Binary file — cannot display' } + const content = await readFile(filePath, 'utf-8') - - // Check for binary content (null bytes in first 8 KB) - const sample = content.slice(0, 8192) - if (sample.includes('\0')) return { error: 'Binary file — cannot display' } - return { content } } catch (err: unknown) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/index.ts` around lines 191 - 211, The handler registered with ipcMain.handle for IpcChannels.FS_READ_FILE currently calls readFile(filePath, 'utf-8') before checking for binary content, which wastes time/memory for large files; change the flow to stat(filePath) as now, then read only an initial sample (e.g., first 8192 bytes) as a Buffer (use fs.read or readFile without encoding or a small createReadStream) and check that Buffer for null bytes before calling readFile(filePath, 'utf-8') to load the full content; keep the MAX_FILE_SIZE check and error returns the same, and ensure the null-byte check uses the sampled Buffer so you only decode the whole file when it’s safe.packages/renderer/src/components/panes/EditorPane.tsx (1)
74-76: Dirty flag persists after undoing all changes.The dirty state is set on any
docChangedevent without checking if the content matches the clean baseline. If a user makes a change and then undos it, the tab remains marked dirty.This is a known UX limitation. A future improvement could compare content to the clean baseline on a debounced basis or when the user attempts to close the tab.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/components/panes/EditorPane.tsx` around lines 74 - 76, The dirty flag is being set on any update.docChanged event in EditorPane.tsx which leaves the tab marked dirty even after undoing all changes; instead, when handling editor updates (the block that calls setDirty(filePath, true)), compare the current document content to the clean baseline and only call setDirty(filePath, true) if they differ (and call setDirty(filePath, false) when they match). Locate the update handler around the docChanged check and use the editor's current document string (or the same clean baseline variable used when the file was loaded) to decide dirty state so undo/redo that restores the baseline clears the dirty flag.IDE_BUILD_PLAN.md (1)
1125-1133: Add blank lines around tables for consistent markdown formatting.The markdownlint tool flags that tables should be surrounded by blank lines for proper rendering.
Proposed fix
Add a blank line before line 1126 (after "### Phase 2: Core IDE Features (Weeks 4–7)"):
### Phase 2: Core IDE Features (Weeks 4–7) + | Milestone | Status | Notes |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@IDE_BUILD_PLAN.md` around lines 1125 - 1133, Add blank lines surrounding the markdown table under the "### Phase 2: Core IDE Features (Weeks 4–7)" header so the table is separated from the heading and following text; specifically insert an empty line immediately after that header and another empty line after the end of the table block (the rows starting with "| Milestone | Status | Notes |" through the last row) to satisfy markdownlint's "tables must be surrounded by blank lines" rule.packages/main/src/ptyManager.ts (1)
35-42: PTY event listeners are not cleaned up when PTY is manually killed.When
PTY_KILLis called, the PTY is killed and removed from the map, but theonDataandonExitlisteners remain attached to the now-dead PTY. While node-pty likely handles this gracefully, explicit cleanup would be cleaner.Proposed approach
Store the disposables returned by
onDataandonExitand call them during kill:-const ptys = new Map<string, IPty>() +interface PtyEntry { + pty: IPty + dispose: () => void +} +const ptys = new Map<string, PtyEntry>() // In PTY_CREATE handler: - ptys.set(id, pty) - - pty.onData((data) => { + const dataDisposable = pty.onData((data) => { getWebContents()?.send(IpcChannels.PTY_DATA_OUT, id, data) }) - - pty.onExit(({ exitCode }) => { + const exitDisposable = pty.onExit(({ exitCode }) => { getWebContents()?.send(IpcChannels.PTY_EXIT, id, exitCode) ptys.delete(id) }) + + ptys.set(id, { + pty, + dispose: () => { + dataDisposable.dispose() + exitDisposable.dispose() + }, + }) // In PTY_KILL handler: - const pty = ptys.get(id) - if (pty) { - pty.kill() + const entry = ptys.get(id) + if (entry) { + entry.dispose() + entry.pty.kill() ptys.delete(id) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/main/src/ptyManager.ts` around lines 35 - 42, When handling PTY kill (the PTY_KILL flow) you need to unregister the event listeners returned by pty.onData and pty.onExit before removing the PTY from the ptys map; capture the disposables when creating listeners in the ptyManager (e.g., store the values returned from pty.onData and pty.onExit alongside the pty in the ptys map or another structure), and call those disposables (dispose/unsubscribe) in the PTY_KILL handler right before/after calling pty.kill and before deleting from ptys so the onData and onExit callbacks are removed and no dangling listeners remain.packages/renderer/src/components/panes/TerminalPane.tsx (1)
78-88: Add guard against disposed terminal in ResizeObserver.The
fitAddon.fit()call could throw if the terminal is disposed during a resize event. While the observer is disconnected during cleanup, there's a small race window.Proposed fix
const observer = new ResizeObserver(() => { + if (destroyed) return fitAddon.fit() if (resizeTimer) clearTimeout(resizeTimer) resizeTimer = setTimeout(() => { + if (destroyed) return const id = ptyIdRef.current if (id) window.api.ptyResize(id, term.cols, term.rows) }, 50) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/components/panes/TerminalPane.tsx` around lines 78 - 88, The ResizeObserver callback must guard against the terminal being disposed before calling fitAddon.fit() and avoid throwing; wrap the fitAddon.fit() call in a try/catch and short-circuit if the terminal is not present or has no valid dimensions (use the existing term reference and term.cols/term.rows checks), and keep the existing pty resize debounce logic (referencing fitAddon.fit, ResizeObserver callback, term, ptyIdRef, hostRef, and window.api.ptyResize) so you catch and ignore any errors from fit when the terminal is disposed during a resize.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 36: Fix the heading typo by changing the heading text "Commiting
conventions" to "Committing conventions" (double “m”) in the CLAUDE.md file;
locate the header string "Commiting conventions" and replace it with the
corrected "Committing conventions" to ensure proper spelling.
In `@eslint.config.mjs`:
- Around line 15-18: The flat-config usage of eslint-plugin-react-hooks is
incorrect: stop spreading reactHooks.configs.recommended.rules and instead
register the plugin and merge the flat config rules from
reactHooks.configs.flat.recommended; update the config block that currently
references reactHooks.configs.recommended.rules to add plugins: { 'react-hooks':
reactHooks } (alongside react) and use
...reactHooks.configs.flat.recommended.rules (or {} fallback) when composing
rules while keeping ...react.configs.recommended.rules and
'react/react-in-jsx-scope': 'off'.
In `@packages/main/package.json`:
- Line 9: The package.json currently pins the native dependency "node-pty":
"^1.1.0" which is incompatible with Electron 41; update the dependency in
package.json to a node-pty release that explicitly supports Electron 41 (or the
latest stable node-pty) and/or add a build step to rebuild native modules for
the Electron ABI (e.g., add a postinstall script that runs electron-rebuild or
npm rebuild --runtime=electron/--target=41) so native binaries are rebuilt
against Electron 41; change the "node-pty" version string and/or add the
postinstall script entry to package.json to ensure proper install and runtime
compatibility.
In `@packages/main/src/index.ts`:
- Line 2: The import statement in packages/main/src/index.ts currently brings in
basename which is not used; remove basename from the import list (leave import {
join } from 'path') so the unused-symbol warning is resolved and imports remain
minimal; verify no other code references basename before removing.
In `@packages/renderer/src/components/AppShell.tsx`:
- Around line 64-65: The tab title extraction using filePath.split('/').pop()
fails on Windows paths; replace this with a platform-aware basename extraction
by using the Node path utility: import path and compute the filename with
path.basename(filePath) (update the const name assignment where 'name' is
currently derived from filePath) so filenames are extracted correctly across
OSes.
In `@packages/renderer/src/components/FileTree/FileTree.tsx`:
- Around line 88-96: The code reads from the stale closure variable nodes after
calling setNodes, which can let two quick clicks both pass the checks and
trigger duplicate window.api.readDir calls; fix by introducing an in-flight
tracking Set (e.g., inFlightLoadsRef) and check/instrument it around the load:
before calling readDir, compute the current path key and if
inFlightLoadsRef.current.has(path) return; otherwise add path to the set, then
proceed with readDir, and finally remove the path from the set in both success
and error paths; alternatively, perform the pre-read checks against a fresh
state via the functional setNodes updater and only start readDir when that
updater confirms not-expanded/not-loaded, referencing the existing nodes,
setNodes, node.isExpanded, node.isLoaded and window.api.readDir identifiers to
locate the logic to change.
In `@packages/renderer/src/components/panes/EditorPane.tsx`:
- Around line 78-91: The Mod-s key handler inside keymap.of currently only
handles successful writeFile responses and ignores failures; update the handler
(the run callback that calls window.api.writeFile) to detect and handle error
cases and promise rejections: on a non-success response or rejected promise,
call an appropriate user-facing notification or alert and log the error, and
ensure cleanContentMap.set(filePath, content) and setDirty(filePath, false) are
only executed on success; keep using the same symbols (keymap.of, Mod-s,
window.api.writeFile, cleanContentMap, setDirty) so the fix is easy to locate.
- Around line 56-57: When restoring from cached state (the branch where `cached`
is assigned to `state`), the view can retain the old theme; after you create the
editor view from that `state` ensure you reconfigure the theme compartment with
the current theme. Specifically, after the view is constructed (where the code
uses the resulting EditorView instance), call view.dispatch({ effects:
themeCompartment.reconfigure(currentThemeExtension) }) (or the equivalent
reconfigure call using your `themeCompartment` and current theme
extension/variable) so the restored view adopts the up-to-date theme instead of
the cached one.
In `@packages/renderer/src/components/panes/TerminalPane.tsx`:
- Around line 61-63: The term.onData subscription in TerminalPane is never
disposed, so store the disposable returned by term.onData (call it e.g.
dataDisposable) and dispose it in the component cleanup (useEffect return or
unmount handler) to avoid writes after unmount; locate the term.onData(...) call
in TerminalPane, assign its return value and call dataDisposable.dispose() when
the component unmounts or when the PTY/id changes, ensuring you still call
window.api.ptyWrite(id, data) only while mounted.
In `@packages/renderer/src/components/Sidebar.tsx`:
- Around line 23-29: The persistWidth debounce can fire after the component
unmounts because debounceTimer.current is never cleared; add a cleanup that
clears the pending timeout on unmount (check debounceTimer.current and call
clearTimeout) so window.api.setSidebarWidth won't run after unmount. Implement
this by adding a useEffect with a cleanup that clears debounceTimer.current (or
incorporate a clear in an existing effect), referencing debounceTimer and the
persistWidth function to ensure the pending timer is cancelled when the Sidebar
component unmounts.
In `@packages/renderer/src/components/WorkspaceRibbon.tsx`:
- Around line 13-21: The current useEffect in WorkspaceRibbon (calling
window.api.getWorkspaceRoot and setWorkspaceName) splits the path using only '/'
which breaks on Windows; update the trimming and splitting logic to handle both
'/' and '\' separators (for example, trim trailing slashes/backslashes and split
on a regex matching either separator) before taking the last segment as the
basename so setWorkspaceName receives a correct cross-platform folder name;
update the logic inside the useEffect that processes the root value accordingly.
In `@packages/renderer/src/lib/editorDirtyState.ts`:
- Line 16: The current iteration over the Set named listeners (in
editorDirtyState.ts) can break if a callback returned by onDirtyChange
unsubscribes itself during iteration; fix this by iterating over a snapshot copy
(e.g., Array.from(listeners) or [...listeners]) instead of the Set directly and
then call each cb(filePath, dirty) from that snapshot to avoid mutation during
iteration.
In `@packages/renderer/src/lib/editorStateCache.ts`:
- Line 3: The exported but unused removeCachedState() indicates dead code and
the Map cache (cache: Map<string, EditorState>) currently grows unbounded;
either remove the unused removeCachedState export and associated logic, or
implement eviction: replace the plain Map with an LRU-backed structure (or wrap
Map with an ordered key list) that enforces a MAX_CACHE_SIZE and evicts the
least-recently-used EditorState on insert, update access patterns (ensure
getCachedState/getOrCreateCachedState update recency), and export
removeCachedState only if it’s needed for explicit invalidation; update usages
referencing cache, getCachedState, and any create/update functions to maintain
recency and enforce eviction.
---
Nitpick comments:
In `@IDE_BUILD_PLAN.md`:
- Around line 1125-1133: Add blank lines surrounding the markdown table under
the "### Phase 2: Core IDE Features (Weeks 4–7)" header so the table is
separated from the heading and following text; specifically insert an empty line
immediately after that header and another empty line after the end of the table
block (the rows starting with "| Milestone | Status | Notes |" through the last
row) to satisfy markdownlint's "tables must be surrounded by blank lines" rule.
In `@packages/main/src/index.ts`:
- Around line 191-211: The handler registered with ipcMain.handle for
IpcChannels.FS_READ_FILE currently calls readFile(filePath, 'utf-8') before
checking for binary content, which wastes time/memory for large files; change
the flow to stat(filePath) as now, then read only an initial sample (e.g., first
8192 bytes) as a Buffer (use fs.read or readFile without encoding or a small
createReadStream) and check that Buffer for null bytes before calling
readFile(filePath, 'utf-8') to load the full content; keep the MAX_FILE_SIZE
check and error returns the same, and ensure the null-byte check uses the
sampled Buffer so you only decode the whole file when it’s safe.
In `@packages/main/src/ptyManager.ts`:
- Around line 35-42: When handling PTY kill (the PTY_KILL flow) you need to
unregister the event listeners returned by pty.onData and pty.onExit before
removing the PTY from the ptys map; capture the disposables when creating
listeners in the ptyManager (e.g., store the values returned from pty.onData and
pty.onExit alongside the pty in the ptys map or another structure), and call
those disposables (dispose/unsubscribe) in the PTY_KILL handler right
before/after calling pty.kill and before deleting from ptys so the onData and
onExit callbacks are removed and no dangling listeners remain.
In `@packages/renderer/src/components/AppShell.tsx`:
- Around line 16-22: The registered handler for registerAppActions captures
onFileOpen at mount, risking a stale closure; change to one of two fixes: either
store the latest onFileOpen in a ref (e.g., create a ref currentOnFileOpen and
update it whenever onFileOpen changes, then register a stable callback that
calls currentOnFileOpen.current), or make the useEffect re-run when onFileOpen
changes (add onFileOpen to the dependency array) so registerAppActions is
re-registered with the new function; update the code around useEffect,
registerAppActions, and onFileOpen accordingly.
In `@packages/renderer/src/components/FileTree/FileTree.tsx`:
- Around line 117-118: Replace the non-null assertion on the Map lookup in
FileTree.tsx by guarding the result of next.get(path) instead of using
"!"—retrieve current via const current = next.get(path) and if current is
undefined skip or continue the loop (or handle a sensible default) before
calling next.set(path, { ...current, isExpanded: true, isLoaded: true, children:
childPaths }); this removes the ESLint non-null assertion while preserving the
existing logic around entries/next.
- Around line 128-129: The non-null assertion on stack.pop() in the FileTree
component's loop should be removed to satisfy ESLint: replace the expression
"const p = stack.pop()!" with a safe guarded pop (e.g., assign without "!" and
early-continue or if-check) so that when stack.pop() returns undefined you
handle it explicitly before using p; update the loop body where p is used (the
variable named p from stack.pop()) to rely on the guard rather than a non-null
assertion.
In `@packages/renderer/src/components/FileTree/FileTreeItem.tsx`:
- Around line 3-9: Extract the duplicated FileTreeNode interface into a single
shared declaration file (e.g., create a new types.ts in the same directory),
export the interface from that file, then update FileTreeItem.tsx and
FileTree.tsx to import FileTreeNode instead of declaring it locally; remove the
local duplicate interface declarations so both components use the shared
FileTreeNode type to ensure a single source of truth.
In `@packages/renderer/src/components/panes/EditorPane.tsx`:
- Around line 74-76: The dirty flag is being set on any update.docChanged event
in EditorPane.tsx which leaves the tab marked dirty even after undoing all
changes; instead, when handling editor updates (the block that calls
setDirty(filePath, true)), compare the current document content to the clean
baseline and only call setDirty(filePath, true) if they differ (and call
setDirty(filePath, false) when they match). Locate the update handler around the
docChanged check and use the editor's current document string (or the same clean
baseline variable used when the file was loaded) to decide dirty state so
undo/redo that restores the baseline clears the dirty flag.
In `@packages/renderer/src/components/panes/EditorTab.tsx`:
- Around line 8-18: Replace the synchronous browser confirm with the app's
themed confirmation modal: modify EditorTab's handleClose to be async and await
a promise-based confirmation dialog instead of using window.confirm, invoke the
app modal (e.g. your ConfirmModal or useModal hook) to show "Discard unsaved
changes?" and resolve true/false, and then call props.api.close() only if the
modal resolved true; update the closeActionOverride prop on DockviewDefaultTab
to pass the new async handleClose and ensure the modal component returns a
boolean so isDirty(filePath) + handleClose logic remains intact.
In `@packages/renderer/src/components/panes/TerminalPane.tsx`:
- Around line 78-88: The ResizeObserver callback must guard against the terminal
being disposed before calling fitAddon.fit() and avoid throwing; wrap the
fitAddon.fit() call in a try/catch and short-circuit if the terminal is not
present or has no valid dimensions (use the existing term reference and
term.cols/term.rows checks), and keep the existing pty resize debounce logic
(referencing fitAddon.fit, ResizeObserver callback, term, ptyIdRef, hostRef, and
window.api.ptyResize) so you catch and ignore any errors from fit when the
terminal is disposed during a resize.
In `@packages/renderer/src/components/StatusBar.tsx`:
- Around line 20-23: The StatusBar component currently renders a hardcoded
branch name "main" next to GitBranchIcon; add a clear TODO comment above the
span (or inside the StatusBar component) indicating this is a placeholder and
must be replaced with dynamic git integration later (e.g., replace with a
branchName prop or a git service lookup); reference the span with class
"status-bar__item--branch" and the GitBranchIcon so reviewers can find the exact
location and include a short tracking note (ticket/issue) or "TODO" tag for
future work.
In `@packages/renderer/src/hooks/useTheme.ts`:
- Around line 15-29: The useEffect currently calls window.api.getTheme() without
handling promise rejection; wrap or chain a .catch on window.api.getTheme() to
handle errors (log the error and fall back to a default theme) and ensure you
still call setTheme(...) and document.documentElement.setAttribute('data-theme',
...) with the fallback value; also ensure window.api.onThemeChanged usage
remains unchanged for updates. Use the existing identifiers (useEffect,
window.api.getTheme, window.api.onThemeChanged, setTheme,
document.documentElement.setAttribute) to locate where to add the .catch and
fallback logic.
In `@packages/shared/package.json`:
- Around line 5-6: The package.json currently exposes TypeScript sources via the
"main" and "types" fields; update these fields to point to the compiled outputs
(e.g., set "main" to the built JS entry like ./dist/index.js and "types" to the
declaration file like ./dist/index.d.ts) and ensure the build step produces
those files (emitDeclarationOnly/tsconfig "declaration": true or your bundler
emits .d.ts) so consumers import runtime JS and type declarations instead of .ts
sources.
In `@packages/shared/src/index.test.ts`:
- Around line 17-23: The test "has string literal values (no accidental
undefined)" validates non-empty strings but misses duplicates; update that test
(the one iterating over Object.values(IpcChannels)) to assert uniqueness by
creating a Set from the values and expecting its size to equal the original
values.length (or otherwise fail if a duplicate exists), so any duplicate IPC
channel string in IpcChannels is caught.
In `@playwright.config.ts`:
- Around line 6-9: The Playwright config sets retries: 0 so trace:
'on-first-retry' will never run; change the trace strategy to capture traces on
failure by updating the trace setting under use (the use.trace option) to
'retain-on-failure' (or another appropriate mode) so traces are collected
without enabling retries; locate the retries property and the use.trace entry in
the Playwright config (playwright.config.ts) and replace the trace value
accordingly.
In `@tests/e2e/launch.test.ts`:
- Around line 8-36: Extract the duplicated electron launch/teardown into shared
test setup: create a beforeEach that launches Electron with the same args/cwd
(the current electron.launch call), assigns the returned object to a shared app
variable and awaits app.firstWindow() assigned to a shared window variable, and
create an afterEach that calls app.close(); then update both tests ('opens a
window with the correct title' and 'renders the app shell content') to use the
shared app/window instead of repeating the
electron.launch/firstWindow/waitForLoadState/close sequence (or alternatively
factor the launch logic into a helper function used by beforeEach).
In `@tests/unit/app.test.tsx`:
- Around line 34-39: The test setup's beforeEach currently defines window.api to
mockApi but doesn't reset mock history; add a call to vi.clearAllMocks() at the
start of the beforeEach so mock call/instance history is reset before each test,
keeping window.api assignment (Object.defineProperty on window, 'api') intact
and ensuring mockApi starts fresh for every test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3f3b36a-a27e-40cd-87d8-a24347f65559
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (58)
.gitignore.prettierrcCLAUDE.mdIDE_BUILD_PLAN.mdelectron.vite.config.tseslint.config.mjspackage.jsonpackages/main/package.jsonpackages/main/src/index.tspackages/main/src/preload.tspackages/main/src/ptyManager.tspackages/main/tsconfig.jsonpackages/renderer/index.htmlpackages/renderer/package.jsonpackages/renderer/src/App.tsxpackages/renderer/src/components/AgentStatusDot.tsxpackages/renderer/src/components/AppShell.tsxpackages/renderer/src/components/DockviewContainer.tsxpackages/renderer/src/components/FileTree/FileTree.tsxpackages/renderer/src/components/FileTree/FileTreeItem.tsxpackages/renderer/src/components/Sidebar.tsxpackages/renderer/src/components/StatusBar.tsxpackages/renderer/src/components/ThemeToggle.tsxpackages/renderer/src/components/WorkspaceRibbon.tsxpackages/renderer/src/components/panes/EditorPane.tsxpackages/renderer/src/components/panes/EditorTab.tsxpackages/renderer/src/components/panes/PlaceholderPane.tsxpackages/renderer/src/components/panes/TerminalPane.tsxpackages/renderer/src/env.d.tspackages/renderer/src/hooks/useEditorStatus.tspackages/renderer/src/hooks/useTheme.tspackages/renderer/src/lib/appActions.tspackages/renderer/src/lib/editorDirtyState.tspackages/renderer/src/lib/editorStateCache.tspackages/renderer/src/lib/editorTheme.tspackages/renderer/src/lib/editorWrap.tspackages/renderer/src/lib/languageExtension.tspackages/renderer/src/lib/terminalTheme.tspackages/renderer/src/main.tsxpackages/renderer/src/styles/app-shell.csspackages/renderer/src/styles/dockview-theme.csspackages/renderer/src/styles/editor-pane.csspackages/renderer/src/styles/file-tree.csspackages/renderer/src/styles/global.csspackages/renderer/src/styles/terminal-pane.csspackages/renderer/src/styles/themes.csspackages/renderer/tsconfig.jsonpackages/shared/package.jsonpackages/shared/src/index.test.tspackages/shared/src/index.tspackages/shared/tsconfig.jsonplaywright.config.tspnpm-workspace.yamltests/e2e/launch.test.tstests/setup.tstests/unit/app.test.tsxtsconfig.jsonvitest.config.ts
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (19)
packages/renderer/src/App.tsx (1)
packages/renderer/src/components/AppShell.tsx (1)
AppShell(9-95)
packages/renderer/src/env.d.ts (1)
packages/shared/src/index.ts (1)
WindowApi(65-102)
packages/renderer/src/main.tsx (3)
packages/renderer/src/hooks/useTheme.ts (1)
ThemeProvider(12-39)packages/renderer/src/hooks/useEditorStatus.ts (1)
EditorStatusProvider(19-31)packages/renderer/src/App.tsx (1)
App(3-5)
packages/renderer/src/components/ThemeToggle.tsx (1)
packages/renderer/src/hooks/useTheme.ts (1)
useTheme(41-45)
packages/renderer/src/components/StatusBar.tsx (1)
packages/renderer/src/hooks/useEditorStatus.ts (1)
useEditorStatus(33-37)
packages/shared/src/index.test.ts (1)
packages/shared/src/index.ts (1)
IpcChannels(8-42)
packages/renderer/src/components/WorkspaceRibbon.tsx (2)
packages/renderer/src/components/AgentStatusDot.tsx (1)
AgentStatusDot(11-18)packages/renderer/src/components/ThemeToggle.tsx (1)
ThemeToggle(20-28)
packages/renderer/src/components/Sidebar.tsx (1)
packages/renderer/src/components/FileTree/FileTree.tsx (1)
FileTree(20-153)
packages/renderer/src/lib/editorTheme.ts (1)
packages/shared/src/index.ts (1)
ThemeName(44-44)
packages/renderer/src/components/panes/EditorTab.tsx (1)
packages/renderer/src/lib/editorDirtyState.ts (1)
isDirty(4-6)
tests/unit/app.test.tsx (4)
packages/shared/src/index.ts (1)
WindowApi(65-102)packages/renderer/src/hooks/useTheme.ts (1)
ThemeProvider(12-39)packages/renderer/src/hooks/useEditorStatus.ts (1)
EditorStatusProvider(19-31)packages/renderer/src/App.tsx (1)
App(3-5)
packages/renderer/src/components/AppShell.tsx (2)
packages/renderer/src/lib/appActions.ts (1)
registerAppActions(8-10)packages/renderer/src/components/DockviewContainer.tsx (1)
DockviewContainer(24-64)
packages/renderer/src/components/FileTree/FileTree.tsx (2)
packages/shared/src/index.ts (1)
DirEntry(46-50)packages/renderer/src/components/FileTree/FileTreeItem.tsx (1)
FileTreeItem(51-77)
packages/main/src/index.ts (2)
packages/shared/src/index.ts (5)
AppSettings(52-56)DEFAULT_SETTINGS(58-62)IpcChannels(8-42)ThemeName(44-44)DirEntry(46-50)packages/main/src/ptyManager.ts (2)
registerPtyHandlers(16-62)killAllPtys(64-69)
packages/renderer/src/components/DockviewContainer.tsx (4)
packages/renderer/src/components/panes/PlaceholderPane.tsx (1)
PlaceholderPane(3-9)packages/renderer/src/components/panes/EditorPane.tsx (1)
EditorPane(22-201)packages/renderer/src/components/panes/TerminalPane.tsx (1)
TerminalPane(12-120)packages/renderer/src/components/panes/EditorTab.tsx (1)
EditorTab(8-18)
packages/renderer/src/components/panes/TerminalPane.tsx (3)
packages/renderer/src/hooks/useTheme.ts (1)
useTheme(41-45)packages/renderer/src/lib/terminalTheme.ts (1)
getXtermTheme(7-31)packages/renderer/src/lib/appActions.ts (1)
getAppActions(12-14)
packages/main/src/ptyManager.ts (1)
packages/shared/src/index.ts (2)
AppSettings(52-56)IpcChannels(8-42)
packages/renderer/src/components/panes/EditorPane.tsx (7)
packages/renderer/src/hooks/useTheme.ts (1)
useTheme(41-45)packages/renderer/src/hooks/useEditorStatus.ts (1)
useEditorStatus(33-37)packages/renderer/src/lib/editorStateCache.ts (2)
getCachedState(5-7)setCachedState(9-11)packages/renderer/src/lib/languageExtension.ts (2)
getLanguageName(38-40)getLanguageExtension(34-36)packages/renderer/src/lib/editorTheme.ts (2)
themeCompartment(7-7)getThemeExtension(20-26)packages/renderer/src/lib/editorWrap.ts (3)
wrapCompartment(5-5)getWrapExtension(18-20)toggleWrap(13-16)packages/renderer/src/lib/editorDirtyState.ts (2)
setDirty(8-17)onDirtyChange(19-22)
packages/main/src/preload.ts (1)
packages/shared/src/index.ts (3)
WindowApi(65-102)IpcChannels(8-42)ThemeName(44-44)
🪛 ESLint
packages/renderer/src/main.tsx
[error] 12-12: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
packages/renderer/src/components/FileTree/FileTree.tsx
[error] 117-117: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
[error] 129-129: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
packages/main/src/index.ts
[error] 2-2: 'basename' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 LanguageTool
CLAUDE.md
[style] ~39-~39: Consider shortening or rephrasing this to strengthen your wording.
Context: ... - Always update IDE_BUILD_PLAN.md when making changes to the codebase. - Use feat: for new fe...
(MAKE_CHANGES)
🪛 markdownlint-cli2 (0.21.0)
IDE_BUILD_PLAN.md
[warning] 1126-1126: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🪛 Stylelint (17.4.0)
packages/renderer/src/styles/global.css
[error] 22-22: Expected "optimizeLegibility" to be "optimizelegibility" (value-keyword-case)
(value-keyword-case)
| - GitHub: https://github.com/Cheezeiii365/aIDE | ||
| - Branch: `main` | ||
|
|
||
| ### Commiting conventions |
Contributor
There was a problem hiding this comment.
Fix heading typo.
Line [36] should be “Committing conventions” (double “m”).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` at line 36, Fix the heading typo by changing the heading text
"Commiting conventions" to "Committing conventions" (double “m”) in the
CLAUDE.md file; locate the header string "Commiting conventions" and replace it
with the corrected "Committing conventions" to ensure proper spelling.
Comment on lines
+15
to
+18
| rules: { | ||
| ...react.configs.recommended.rules, | ||
| ...reactHooks.configs.recommended.rules, | ||
| 'react/react-in-jsx-scope': 'off', |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
eslint-plugin-react-hooks flat config recommended rules export
💡 Result:
To use the recommended flat config rules from eslint-plugin-react-hooks in ESLint's flat config format (eslint.config.js), import the plugin and use reactHooks.configs.flat.recommended. Example: import reactHooks from 'eslint-plugin-react-hooks'; // Optional: import { defineConfig } from 'eslint/config'; // if using defineConfig export default [ // Other configs... reactHooks.configs.flat.recommended, ]; This exports an array of FlatConfig objects that includes the plugin registration and all recommended rules enabled, such as: - 'react-hooks/rules-of-hooks': 'error' - 'react-hooks/exhaustive-deps': 'warn' - 'react-hooks/config': 'error' - 'react-hooks/error-boundaries': 'error' - 'react-hooks/component-hook-factories': 'error' - 'react-hooks/gating': 'error' - 'react-hooks/globals': 'error' - 'react-hooks/immutability': 'error' - 'react-hooks/preserve-manual-memoization': 'error' - 'react-hooks/purity': 'error' - And additional React Compiler rules like 'react-hooks/refs', 'react-hooks/set-state-in-effect', etc. For bleeding-edge experimental rules, use reactHooks.configs.flat['recommended-latest'] instead (deprecated in favor of recommended post-v6.0). This is the official method per the plugin's README (updated Feb 2025 via PR #32457), npm page, and react.dev docs. Legacy .eslintrc uses "plugin:react-hooks/recommended".
Citations:
- 1: https://github.com/facebook/react/blob/main/packages/eslint-plugin-react-hooks/README.md
- 2: https://www.npmjs.com/package/eslint-plugin-react-hooks
- 3: https://react.dev/reference/eslint-plugin-react-hooks
- 4: feat(eslint-plugin-react-hooks): make flat config the
recommendedconfig facebook/react#32457 - 5: https://www.grilljs.com
- 6: https://beta.reactjs.org/reference/eslint-plugin-react-hooks
Fix eslint-plugin-react-hooks flat config usage.
The current code uses reactHooks.configs.recommended.rules, but the flat config API exports reactHooks.configs.flat.recommended as a complete config object (including plugin registration and rules). For flat config, replace the spread of reactHooks.configs.recommended.rules with the appropriate flat config structure. Example:
Suggested fix
export default [
// ... other configs
{
files: ['packages/renderer/**/*.{jsx,tsx}'],
languageOptions: {
parserOptions: {
ecmaFeatures: {
jsx: true,
},
},
},
plugins: {
react: react,
'react-hooks': reactHooks,
},
rules: {
...react.configs.recommended.rules,
...reactHooks.configs.flat.recommended.rules || {},
'react/react-in-jsx-scope': 'off',
},
},
];Reference: eslint-plugin-react-hooks documentation
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@eslint.config.mjs` around lines 15 - 18, The flat-config usage of
eslint-plugin-react-hooks is incorrect: stop spreading
reactHooks.configs.recommended.rules and instead register the plugin and merge
the flat config rules from reactHooks.configs.flat.recommended; update the
config block that currently references reactHooks.configs.recommended.rules to
add plugins: { 'react-hooks': reactHooks } (alongside react) and use
...reactHooks.configs.flat.recommended.rules (or {} fallback) when composing
rules while keeping ...react.configs.recommended.rules and
'react/react-in-jsx-scope': 'off'.
| "dependencies": { | ||
| "@aide/shared": "workspace:*", | ||
| "electron-store": "^6.0.1", | ||
| "node-pty": "^1.1.0" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does node-pty version 1.1.0 explicitly support Electron 41 (including ABI/prebuild guidance)?
💡 Result:
No, node-pty version 1.1.0 does not explicitly support Electron 41, including ABI/prebuild guidance. Official documentation states support for Node.js 16 or Electron 19 minimum, with versions tied to Visual Studio Code's usage, but no specific mention of Electron 41 in release notes, changelogs, tags, or prebuild lists for v1.1.0. GitHub tags and releases show v1.1.0-beta versions and later v1.2.0-betas, but no stable v1.1.0 release explicitly lists Electron 41. Issues discuss compatibility up to Electron 32-33 with rebuild issues, not 41. Prebuild forks support up to certain versions but not confirming 41 for official 1.1.0. Users must rebuild manually using node-gyp for Electron 41 ABI compatibility, as no prebuilt binaries or explicit guidance found.
Citations:
- 1: https://www.npmjs.com/package/node-pty/v/1.1.0-beta22
- 2: https://github.com/microsoft/node-pty
- 3: https://github.com/microsoft/node-pty/releases
- 4: https://github.com/microsoft/node-pty/tags
- 5: node-pty does not work with latest versions of electron microsoft/node-pty#728
- 6: node-pty hangs after PR #653 (v1.1.0-beta13) when writing multiple chunks rapidly microsoft/node-pty#797
🏁 Script executed:
cd packages/main && cat package.json | head -30Repository: Cheezeiii365/aIDE
Length of output: 274
🏁 Script executed:
cd / && find . -maxdepth 2 -name "package.json" -type f -exec head -50 {} + 2>/dev/null | head -100Repository: Cheezeiii365/aIDE
Length of output: 43
🏁 Script executed:
git ls-files package.json | head -5Repository: Cheezeiii365/aIDE
Length of output: 73
🏁 Script executed:
cat package.json | head -100Repository: Cheezeiii365/aIDE
Length of output: 1687
Address node-pty version incompatibility with Electron 41.
Line 9 pins node-pty@^1.1.0, a native module that does not support Electron 41. Official support for node-pty v1.1.0 only extends to Electron 19, with known compatibility issues reported up to Electron 32–33. This version lacks prebuilt binaries for Electron 41 ABI, requiring manual rebuilding via node-gyp during installation, which may cause runtime failures if not handled. Either upgrade to a newer node-pty version with explicit Electron 41 support or ensure your build process properly rebuilds native modules for the target Electron ABI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/main/package.json` at line 9, The package.json currently pins the
native dependency "node-pty": "^1.1.0" which is incompatible with Electron 41;
update the dependency in package.json to a node-pty release that explicitly
supports Electron 41 (or the latest stable node-pty) and/or add a build step to
rebuild native modules for the Electron ABI (e.g., add a postinstall script that
runs electron-rebuild or npm rebuild --runtime=electron/--target=41) so native
binaries are rebuilt against Electron 41; change the "node-pty" version string
and/or add the postinstall script entry to package.json to ensure proper install
and runtime compatibility.
| @@ -0,0 +1,242 @@ | |||
| import { app, BaseWindow, WebContentsView, ipcMain, Menu, dialog } from 'electron' | |||
| import { join, basename } from 'path' | |||
Contributor
There was a problem hiding this comment.
Remove unused basename import.
Static analysis correctly flags that basename is imported but never used.
Proposed fix
-import { join, basename } from 'path'
+import { join } from 'path'📝 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
| import { join, basename } from 'path' | |
| import { join } from 'path' |
🧰 Tools
🪛 ESLint
[error] 2-2: 'basename' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/main/src/index.ts` at line 2, The import statement in
packages/main/src/index.ts currently brings in basename which is not used;
remove basename from the import list (leave import { join } from 'path') so the
unused-symbol warning is resolved and imports remain minimal; verify no other
code references basename before removing.
Comment on lines
+64
to
+65
| // Extract filename for tab title | ||
| const name = filePath.split('/').pop() ?? filePath |
Contributor
There was a problem hiding this comment.
Cross-platform path handling for tab title.
Same issue as in WorkspaceRibbon.tsx — this won't correctly extract filenames on Windows.
🛠️ Proposed fix
// Extract filename for tab title
- const name = filePath.split('/').pop() ?? filePath
+ const name = filePath.split(/[\\/]/).pop() ?? filePath📝 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
| // Extract filename for tab title | |
| const name = filePath.split('/').pop() ?? filePath | |
| // Extract filename for tab title | |
| const name = filePath.split(/[\\/]/).pop() ?? filePath |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/renderer/src/components/AppShell.tsx` around lines 64 - 65, The tab
title extraction using filePath.split('/').pop() fails on Windows paths; replace
this with a platform-aware basename extraction by using the Node path utility:
import path and compute the filename with path.basename(filePath) (update the
const name assignment where 'name' is currently derived from filePath) so
filenames are extracted correctly across OSes.
Comment on lines
+61
to
+63
| term.onData((data) => { | ||
| window.api.ptyWrite(id, data) | ||
| }) |
Contributor
There was a problem hiding this comment.
Missing cleanup for term.onData subscription.
The term.onData callback creates a subscription that isn't tracked for cleanup. If the component unmounts while the PTY is still active, this listener will continue attempting to write to the PTY.
Proposed fix
+ let cleanupTermData: (() => void) | null = null
// ... inside init() after ptyIdRef.current = id ...
- term.onData((data) => {
+ const termDataDisposable = term.onData((data) => {
window.api.ptyWrite(id, data)
})
+ cleanupTermData = () => termDataDisposable.dispose()
// ... in cleanup ...
return () => {
destroyed = true
observer.disconnect()
if (resizeTimer) clearTimeout(resizeTimer)
+ cleanupTermData?.()
cleanupData?.()
cleanupExit?.()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/renderer/src/components/panes/TerminalPane.tsx` around lines 61 -
63, The term.onData subscription in TerminalPane is never disposed, so store the
disposable returned by term.onData (call it e.g. dataDisposable) and dispose it
in the component cleanup (useEffect return or unmount handler) to avoid writes
after unmount; locate the term.onData(...) call in TerminalPane, assign its
return value and call dataDisposable.dispose() when the component unmounts or
when the PTY/id changes, ensuring you still call window.api.ptyWrite(id, data)
only while mounted.
Comment on lines
+23
to
+29
| // Debounced save of sidebar width | ||
| const persistWidth = useCallback((w: number) => { | ||
| if (debounceTimer.current) clearTimeout(debounceTimer.current) | ||
| debounceTimer.current = setTimeout(() => { | ||
| window.api.setSidebarWidth(w) | ||
| }, 500) | ||
| }, []) |
Contributor
There was a problem hiding this comment.
Debounce timer not cleared on unmount.
If the component unmounts while a resize debounce is pending, the timer continues and may call setSidebarWidth after unmount. Consider clearing it in a cleanup effect.
🛠️ Proposed fix
+ // Clear debounce timer on unmount
+ useEffect(() => {
+ return () => {
+ if (debounceTimer.current) clearTimeout(debounceTimer.current)
+ }
+ }, [])
// Debounced save of sidebar width
const persistWidth = useCallback((w: number) => {📝 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
| // Debounced save of sidebar width | |
| const persistWidth = useCallback((w: number) => { | |
| if (debounceTimer.current) clearTimeout(debounceTimer.current) | |
| debounceTimer.current = setTimeout(() => { | |
| window.api.setSidebarWidth(w) | |
| }, 500) | |
| }, []) | |
| // Clear debounce timer on unmount | |
| useEffect(() => { | |
| return () => { | |
| if (debounceTimer.current) clearTimeout(debounceTimer.current) | |
| } | |
| }, []) | |
| // Debounced save of sidebar width | |
| const persistWidth = useCallback((w: number) => { | |
| if (debounceTimer.current) clearTimeout(debounceTimer.current) | |
| debounceTimer.current = setTimeout(() => { | |
| window.api.setSidebarWidth(w) | |
| }, 500) | |
| }, []) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/renderer/src/components/Sidebar.tsx` around lines 23 - 29, The
persistWidth debounce can fire after the component unmounts because
debounceTimer.current is never cleared; add a cleanup that clears the pending
timeout on unmount (check debounceTimer.current and call clearTimeout) so
window.api.setSidebarWidth won't run after unmount. Implement this by adding a
useEffect with a cleanup that clears debounceTimer.current (or incorporate a
clear in an existing effect), referencing debounceTimer and the persistWidth
function to ensure the pending timer is cancelled when the Sidebar component
unmounts.
Comment on lines
+13
to
+21
| useEffect(() => { | ||
| window.api.getWorkspaceRoot().then((root) => { | ||
| if (root) { | ||
| // Extract basename from the path | ||
| const parts = root.replace(/\/+$/, '').split('/') | ||
| setWorkspaceName(parts[parts.length - 1] || root) | ||
| } | ||
| }) | ||
| }, []) |
Contributor
There was a problem hiding this comment.
Cross-platform path handling issue.
The path splitting uses only / as a delimiter, which won't correctly extract the basename on Windows where paths use \.
🛠️ Proposed fix for cross-platform support
useEffect(() => {
window.api.getWorkspaceRoot().then((root) => {
if (root) {
// Extract basename from the path
- const parts = root.replace(/\/+$/, '').split('/')
+ const parts = root.replace(/[\\/]+$/, '').split(/[\\/]/)
setWorkspaceName(parts[parts.length - 1] || root)
}
})
}, [])📝 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
| useEffect(() => { | |
| window.api.getWorkspaceRoot().then((root) => { | |
| if (root) { | |
| // Extract basename from the path | |
| const parts = root.replace(/\/+$/, '').split('/') | |
| setWorkspaceName(parts[parts.length - 1] || root) | |
| } | |
| }) | |
| }, []) | |
| useEffect(() => { | |
| window.api.getWorkspaceRoot().then((root) => { | |
| if (root) { | |
| // Extract basename from the path | |
| const parts = root.replace(/[\\/]+$/, '').split(/[\\/]/) | |
| setWorkspaceName(parts[parts.length - 1] || root) | |
| } | |
| }) | |
| }, []) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/renderer/src/components/WorkspaceRibbon.tsx` around lines 13 - 21,
The current useEffect in WorkspaceRibbon (calling window.api.getWorkspaceRoot
and setWorkspaceName) splits the path using only '/' which breaks on Windows;
update the trimming and splitting logic to handle both '/' and '\' separators
(for example, trim trailing slashes/backslashes and split on a regex matching
either separator) before taking the last segment as the basename so
setWorkspaceName receives a correct cross-platform folder name; update the logic
inside the useEffect that processes the root value accordingly.
| } else { | ||
| dirtyMap.delete(filePath) | ||
| } | ||
| for (const cb of listeners) cb(filePath, dirty) |
Contributor
There was a problem hiding this comment.
Potential issue if listeners unsubscribe during iteration.
If a callback invokes its own unsubscribe function (returned by onDirtyChange) during iteration, the Set iteration may behave unexpectedly. Consider iterating over a snapshot:
🛡️ Proposed fix
- for (const cb of listeners) cb(filePath, dirty)
+ for (const cb of [...listeners]) cb(filePath, dirty)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/renderer/src/lib/editorDirtyState.ts` at line 16, The current
iteration over the Set named listeners (in editorDirtyState.ts) can break if a
callback returned by onDirtyChange unsubscribes itself during iteration; fix
this by iterating over a snapshot copy (e.g., Array.from(listeners) or
[...listeners]) instead of the Set directly and then call each cb(filePath,
dirty) from that snapshot to avoid mutation during iteration.
…ache Remove the unused removeCachedState function and implement proper LRU cache eviction with a 64-entry cap to prevent unbounded memory growth. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 25, 2026
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.
Summary
Test plan
pnpm install && pnpm buildcompiles without errorspnpm devlaunches Electron window with sidebar, editor, and terminal panespnpm testpasses all unit tests🤖 Generated with Claude Code
Summary by CodeRabbit