-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: Add LadybugDB installation checks and platform-specific memory paths #1262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
feat: Add LadybugDB installation checks and platform-specific memory paths #1262
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a platform-aware memories directory IPC and renderer hook, surfaces LadybugDB install checks in the memory service and UI, extends terminal command handling to be Windows PowerShell-aware, introduces DPI-aware terminal font sizing, and adds English/French i18n keys for memory/Ladybug. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant Preload
participant Main
participant ConfigPaths
Renderer->>Preload: window.electronAPI.getMemoriesDir()
Preload->>Main: ipcRenderer.invoke(IPC_CHANNELS.MEMORY_GET_DIR)
Main->>ConfigPaths: getMemoriesDir() / getDefaultDbPath()
ConfigPaths-->>Main: platform-specific path
Main-->>Preload: IPC response { success, data: path }
Preload-->>Renderer: resolve Promise with path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Summary of ChangesHello @mateuszruszkowski, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the developer experience for Windows users by resolving two critical issues. It ensures that Windows Bash sessions have complete access to system-wide tools by correctly propagating the full system PATH, and it introduces dynamic DPI-aware font scaling for the terminal, providing a consistent and readable interface across various display configurations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Thanks for your first PR!
A maintainer will review it soon. Please make sure:
- Your branch is synced with
develop - CI checks pass
- You've followed our contribution guide
Welcome to the Auto Claude community!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces two valuable improvements for Windows users: providing the full system PATH to Bash sessions and implementing DPI-aware font scaling in the terminal. These changes will significantly enhance the developer experience.
My review focuses on refining the implementation. I've suggested reusing an existing helper function in the Python backend to simplify the PATH retrieval logic. For the frontend, I've identified a bug in the DPI change handler that prevents it from detecting subsequent DPI changes and proposed a fix. I've also pointed out some opportunities to improve code clarity and reduce duplication in the font scaling calculations. Overall, great work on tackling these platform-specific issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/backend/core/platform/__init__.py`:
- Around line 519-535: The get_windows_system_path function duplicates
case-insensitive env lookup; replace the manual
os.environ.get("PATH")/("Path")/("path") logic by calling the existing helper
get_env_var("PATH") so behavior is centralized; update the return to use
get_env_var("PATH") inside get_windows_system_path and keep the early
is_windows() check intact.
In `@apps/frontend/src/main/env-utils.ts`:
- Around line 111-127: COMMON_BIN_PATHS currently contains hardcoded Windows
roots; replace those literals by constructing Windows paths from environment
variables and platform helpers: when process.platform === 'win32', read
ProgramFiles, ProgramFiles(x86) (or fallback to ProgramFiles), ProgramData,
SystemRoot, LOCALAPPDATA, APPDATA and USERPROFILE, use path.join or path.resolve
to build the specific subpaths (e.g. join(ProgramFiles, 'Git', 'cmd'),
join(ProgramFiles, 'nodejs'), join(LOCALAPPDATA, 'Programs', 'nodejs'),
join(APPDATA, 'npm'), join(USERPROFILE, '.local', 'bin'), join(ProgramData,
'chocolatey', 'bin'), etc.), expand '~' against USERPROFILE for POSIX-style
entries, filter out undefined or non-existent env values, and only include these
constructed entries in COMMON_BIN_PATHS so platform-specific roots are not
hardcoded.
In `@apps/frontend/src/renderer/components/terminal/useXterm.ts`:
- Around line 333-372: Guard against window.matchMedia being undefined and add a
resize fallback, then re-register the media query whenever DPI changes: inside
the effect, check if window.matchMedia exists before using it and fall back to
listening to window.resize; modify the handleDPIChange routine (and its
debounce) to, after adjusting xtermRef.current.options.fontSize and calling
fitAddonRef.current.fit() (using terminalRef to check bounds), recreate the
mediaQuery using the latest window.devicePixelRatio and reattach its 'change'
listener so the query isn't stale; ensure you remove the correct listeners on
cleanup (use removeEventListener when added and removeListener fallback for
older APIs) and reference the existing symbols handleDPIChange, xtermRef,
fitAddonRef, terminalRef, and mediaQuery when updating the logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
🤖 Auto Claude PR ReviewMerge Verdict: 🔴 BLOCKED
Blocked: 4 CI check(s) failing. Fix CI before merge. Risk Assessment
🚨 Blocking Issues (Must Fix)
Generated by Auto Claude PR Review This review identified blockers that must be resolved before merge. Generated by Auto Claude. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/frontend/src/renderer/components/terminal/useXterm.ts`:
- Around line 383-399: Guard against window.matchMedia being undefined and make
the DPI listener re-register the media query after DPI changes: in the effect
that creates mediaQuery (the variable mediaQuery) check if typeof
window.matchMedia === 'function' before using it and return early if not (or add
the provided JSDOM mock in test setup to unblock CI); modify handleDPIChange so
it removes listeners from the existing mediaQuery, recomputes a new mediaQuery
using the current window.devicePixelRatio, and attaches the appropriate listener
via addEventListener/addListener (and corresponding
removeEventListener/removeListener) so subsequent DPI changes (e.g., moving
between monitors) are detected; ensure cleanup returns a function that removes
listeners from whichever mediaQuery is currently registered.
♻️ Duplicate comments (2)
apps/frontend/src/renderer/components/terminal/useXterm.ts (2)
54-56: Comment does not match implementation.The comment describes direct inverse scaling (
1 / 1.5), but the code uses1 / Math.sqrt(scaleFactor). Update the comment to accurately reflect the square-root-based formula.
364-368: Extract duplicated font size calculation to a helper function.This calculation is duplicated from lines 52-58. Extract to a reusable helper to follow DRY and ensure consistency if the formula changes.
♻️ Suggested refactor
Add a helper function at the top of the file (after imports):
/** Calculate DPI-adjusted font size for terminal */ function getAdjustedFontSize(baseFontSize = 13, minFontSize = 11): number { const scaleFactor = window.devicePixelRatio || 1; const adjustedFontSize = scaleFactor > 1 ? Math.max(baseFontSize * (1 / Math.sqrt(scaleFactor)), minFontSize) : baseFontSize; return Math.round(adjustedFontSize); }Then use it in both locations:
- const baseFontSize = 13; - const scaleFactor = window.devicePixelRatio || 1; - const adjustedFontSize = scaleFactor > 1 - ? Math.max(baseFontSize * (1 / Math.sqrt(scaleFactor)), 11) - : baseFontSize; + const adjustedFontSize = getAdjustedFontSize();
53d3373 to
931badd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/frontend/src/renderer/lib/mocks/infrastructure-mock.ts (1)
82-82: Hardcoded Unix path in mock data.Line 82 contains
/usr/local/bin/ollama, which is a Unix-specific path. While this is mock data and not execution logic, consider using a platform-agnostic placeholder or noting this is a Unix-only mock. As per coding guidelines, platform-specific paths should be avoided.apps/frontend/src/main/memory-service.ts (1)
144-146: Use platform abstraction instead ofprocess.platform.Per coding guidelines, avoid checking
process.platformdirectly. Import and useisWindows()from the platform module instead.♻️ Proposed fix
Add the import at the top:
+import { isWindows } from './platform';Then update the code:
- const venvPython = process.platform === 'win32' - ? path.join(backendPath, '.venv', 'Scripts', 'python.exe') - : path.join(backendPath, '.venv', 'bin', 'python'); + const venvPython = isWindows() + ? path.join(backendPath, '.venv', 'Scripts', 'python.exe') + : path.join(backendPath, '.venv', 'bin', 'python');apps/frontend/src/renderer/components/onboarding/GraphitiStep.tsx (1)
828-898: Replace newly added user-facing strings with i18n keys.
These newly introduced UI strings should usereact-i18nextkeys rather than hardcoded text.As per coding guidelines, and based on learnings, this only flags the newly introduced strings.✅ Proposed fix (apply to new strings)
-import { useState, useEffect } from 'react'; +import { useState, useEffect } from 'react'; +import { useTranslation } from 'react-i18next'; export function GraphitiStep({ onNext, onBack, onSkip }: GraphitiStepProps) { + const { t } = useTranslation(); const { settings, updateSettings } = useSettingsStore(); ... - LadybugDB Not Installed + {t('onboarding:graphiti.ladybugNotInstalled.title')} ... - Download Visual Studio Build Tools + {t('onboarding:graphiti.ladybugNotInstalled.downloadBuildTools')} ... - After installing build tools, restart the application to retry. + {t('onboarding:graphiti.ladybugNotInstalled.restartNotice')} ... - Database will be created automatically + {t('onboarding:graphiti.databaseWillBeCreated.title')} ... - LadybugDB uses an embedded database - no Docker required. - The database will be created when you first use memory features. + {t('onboarding:graphiti.databaseWillBeCreated.body')} ... - LadybugDB Ready + {t('onboarding:graphiti.ladybugReady.title')} ... - Memory database is installed and available. + {t('onboarding:graphiti.ladybugReady.body')} ... - Stored in {memoriesDir || 'memories directory'} + {t('onboarding:graphiti.storedIn', { + path: memoriesDir || t('onboarding:graphiti.memoriesDirFallback') + })}Also applies to: 994-995
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/memory-service.ts`:
- Around line 779-784: The code uses a dynamic require to get spawnSync; replace
that with a static import at the top of the module alongside spawn from
'child_process' and remove the require call where spawnSync is used (the block
that currently declares const { spawnSync } = require('child_process'); and then
calls spawnSync). Update imports to statically import { spawn, spawnSync } from
'child_process' so both symbols are available for the code that calls
spawnSync(cmd, checkArgs, ...).
In
`@apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts`:
- Around line 646-691: Add a Linux-specific unit test to the escapeShellCommand
suite: inside the existing describe('escapeShellCommand') block add a test that
calls mockPlatform('linux'), imports escapeShellCommand from
../claude-integration-handler, calls escapeShellCommand('/usr/local/bin/claude')
and asserts the result equals "'/usr/local/bin/claude'"; this mirrors the macOS
(darwin) case and ensures Linux coverage alongside Windows and macOS.
In `@apps/frontend/src/main/terminal/claude-integration-handler.ts`:
- Around line 97-120: The PowerShell branch in escapeShellCommand should use
PowerShell-safe single-quote quoting instead of escapeForWindowsDoubleQuote:
when isWindows() and shellType === 'powershell', produce a single-quoted string
(handle embedded single quotes by doubling them) and prefix with the call
operator (&) so the result is & '...'; do not apply cmd.exe-specific percent (%)
escaping or double-quote escaping (escapeForWindowsDoubleQuote) in this
branch—leave escapeForWindowsDoubleQuote for the cmd.exe branch only.
In `@apps/frontend/src/renderer/components/project-settings/SecuritySettings.tsx`:
- Around line 57-76: Extract the duplicated memoriesDir state and fetch logic
into a reusable hook (e.g., useMemoriesDir) and replace the local
useState/useEffect in SecuritySettings, MemoryBackendSection, and MemoriesTab
with that hook; specifically, move the window.electronAPI.getMemoriesDir() call
and the setMemoriesDir updater into the new useMemoriesDir hook and have it
return the memoriesDir string, then in the components remove the local
memoriesDir/setMemoriesDir and import/use useMemoriesDir() where the current
memoriesDir variable is referenced.
♻️ Duplicate comments (2)
apps/frontend/src/renderer/components/project-settings/MemoryBackendSection.tsx (1)
51-52: Duplicate memoriesDir fetch logic.This is the same pattern as in
SecuritySettings.tsx. As noted in the previous file review, consider extracting this to a shareduseMemoriesDir()hook.Also applies to: 96-107
apps/frontend/src/renderer/components/context/MemoriesTab.tsx (1)
82-95: Duplicate memoriesDir fetch logic.Same pattern as in
SecuritySettings.tsxandMemoryBackendSection.tsx. A shareduseMemoriesDir()hook would eliminate this duplication.
apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts
Show resolved
Hide resolved
apps/frontend/src/renderer/components/project-settings/SecuritySettings.tsx
Outdated
Show resolved
Hide resolved
AndyMik90
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 Auto Claude PR Review
🔴 Follow-up Review: Blocked
🔴 Blocked - 4 CI check(s) failing. Fix CI before merge.
Resolution Status
- ✅ Resolved: 0 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 5 new findings in recent changes
🚨 Blocking Issues
- quality: CI BLOCKER: Missing null check for window.matchMedia causes test failures in JSDOM
- quality: DPI change listener will not fire on subsequent DPI changes after the first
- quality: [FROM COMMENTS] CI Blocker: window.matchMedia is undefined in JSDOM test environment
Verdict
BLOCKED: 4 CI checks failing (test-frontend on all platforms + CI Complete).
The root cause is identified: window.matchMedia is called without a guard at useXterm.ts:386, which throws TypeError: window.matchMedia is not a function in JSDOM test environment.
Issues requiring resolution:
- CRITICAL (CI Blocker): Guard
window.matchMediacall with existence check (NEW-001/CMT-001) - HIGH: Fix stale media query - must re-register listener after DPI changes (NEW-002)
- LOW: Consider adjusting minimum font size for high-DPI displays (NEW-003)
Cross-agent validation: Both new-code-reviewer and comment-analyzer independently identified the same blocking issues, providing high confidence these are genuine bugs.
To unblock: Add guard before matchMedia call, e.g.: if (typeof window.matchMedia !== 'function') return;
Review Process
Agents invoked: new-code-reviewer, comment-analyzer
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (5 selected of 5 total)
🔴 [NEW-001] [CRITICAL] CI BLOCKER: Missing null check for window.matchMedia causes test failures in JSDOM
📁 apps/frontend/src/renderer/components/terminal/useXterm.ts:386
The code calls window.matchMedia() without first checking if it exists. JSDOM (used in tests) does not implement matchMedia, causing TypeError: window.matchMedia is not a function. This is the confirmed cause of the CI failure across all test platforms.
Suggested fix:
Add a guard check before calling matchMedia:
```typescript
if (typeof window === 'undefined' || typeof window.matchMedia !== 'function') {
return;
}
const mediaQuery = window.matchMedia(...);
#### 🟠 [NEW-002] [HIGH] DPI change listener will not fire on subsequent DPI changes after the first
📁 `apps/frontend/src/renderer/components/terminal/useXterm.ts:386`
The media query is created once with the current devicePixelRatio value. When the DPI changes and the handler fires, the old media query no longer matches the new resolution, so it will only fire once. Subsequent DPI changes (e.g., moving between monitors) won't trigger the handler.
**Suggested fix:**
After handling a DPI change, create a new media query with the updated devicePixelRatio using a recursive pattern that re-registers with the new value.
#### 🔵 [NEW-003] [LOW] DPI scaling formula may produce uncomfortably small fonts on high-DPI displays
📁 `apps/frontend/src/renderer/components/terminal/useXterm.ts:57`
The formula baseFontSize * (1 / Math.sqrt(scaleFactor)) with a minimum of 11px may produce small fonts on high-DPI displays. For 200% scaling (devicePixelRatio = 2), calculation yields ~9.19px clamped to 11px, which may be too small for comfortable terminal use.
**Suggested fix:**
Consider using a minimum font size of 12px instead of 11px, or a less aggressive scaling formula.
#### 🔵 [NEW-004] [LOW] Missing cleanup return in fallback path when event listener APIs are unavailable
📁 `apps/frontend/src/renderer/components/terminal/useXterm.ts:360`
The useEffect for DPI handling has three code paths but the implicit fallback path (when neither addEventListener nor addListener exists) does not return a cleanup function, leading to an inconsistent return type.
**Suggested fix:**
Add an explicit return statement: return () => {}; at the end of the useEffect.
#### 🔴 [CMT-001] [CRITICAL] [FROM COMMENTS] CI Blocker: window.matchMedia is undefined in JSDOM test environment
📁 `apps/frontend/src/renderer/components/terminal/useXterm.ts:386`
Both CodeRabbit and Sentry identified that the matchMedia call at line 386 is not guarded. In JSDOM test environments, window.matchMedia is undefined, causing TypeError. This breaks ALL frontend tests on macOS, Windows, and Ubuntu CI runners.
**Suggested fix:**
Guard the matchMedia call: if (typeof window !== 'undefined' && window.matchMedia) { ... } or add matchMedia mock to test setup
---
*This review was generated by Auto Claude.*
931badd to
537cc96
Compare
- Add JSDOM guard for window.matchMedia to fix test failures
- Re-register DPI media query listener after changes (MDN pattern)
- Fix DPI scaling comment to match sqrt() implementation
- Change PowerShell escaping to use single quotes (safer, no expansion)
- Replace dynamic require('child_process') with static spawnSync import
- Add Linux test coverage for escapeShellCommand
- Update PowerShell tests to use single quote expectations
Addresses review comments from CodeRabbit, Gemini Code Assist, and maintainer.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
537cc96 to
7d265a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/frontend/src/renderer/components/project-settings/InfrastructureStatus.tsx (2)
17-64: Avoid showing “Not Installed” when status is unknown.
infrastructureStatuscan be null; in that caseladybugInstalledisundefinedand the UI currently renders a warning state. Consider gating the LadybugDB section until memory status is available to avoid misleading users.🧯 Suggested guard
- const ladybugInstalled = infrastructureStatus?.memory.ladybugInstalled; - const ladybugError = infrastructureStatus?.memory.ladybugError; + const memoryStatus = infrastructureStatus?.memory; + const ladybugInstalled = memoryStatus?.ladybugInstalled; + const ladybugError = memoryStatus?.ladybugError; ... - {/* LadybugDB Installation Status */} - <div className="flex items-center justify-between"> + {/* LadybugDB Installation Status */} + {memoryStatus && ( + <div className="flex items-center justify-between"> ... - </div> + </div> + )} ... - {!ladybugInstalled && ladybugError && ( + {memoryStatus && !ladybugInstalled && ladybugError && (
29-103: Localize all new user-facing strings in this component.The hardcoded status text, labels, and messages are user-facing strings that must use i18n translation keys per the coding guidelines. Add
useTranslation()and replace all hardcoded strings with i18n keys using thememory:infrastructure.*namespace. Note that dynamic error messages from the system (likeladybugErrorandinfrastructureStatus.memory.error) should display as-is without translation keys, since they originate from backend/system sources.🔧 Example pattern
- <span className="text-xs text-foreground">LadybugDB</span> + <span className="text-xs text-foreground">{t('memory:infrastructure.ladybugdb')}</span> ... - <span className="text-xs text-success">Installed</span> + <span className="text-xs text-success">{t('memory:infrastructure.installed')}</span> ... - <span className="text-xs text-warning">Not Installed</span> + <span className="text-xs text-warning">{t('memory:infrastructure.notInstalled')}</span> ... - <span className="text-xs text-muted-foreground">Requires LadybugDB</span> + <span className="text-xs text-muted-foreground">{t('memory:infrastructure.requiresLadybugDb')}</span> ... - Graph memory requires Python 3.12+ with LadybugDB. No Docker needed. + {t('memory:infrastructure.requiresPythonLadybugDb')} ... - Download Visual Studio Build Tools + {t('memory:infrastructure.downloadVsBuildTools')}
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/memory-service.ts`:
- Around line 747-828: The checkLadybugInstalled function currently calls
spawnSync without the Python environment and uses hardcoded user-facing strings;
update the spawnSync call in checkLadybugInstalled to include the env option set
to getMemoryPythonEnv() (same pattern as other spawn() calls) so PYTHONPATH is
correct, and replace all hardcoded error/user messages returned via
ladybugInstallCache (e.g., the "Python not found..." message and the messages
set when ModuleNotFoundError/WinError2/other failures occur) to use i18n
translation keys (e.g., errors:ladybug.pythonNotFound,
errors:ladybug.notInstalled, errors:ladybug.buildTools,
errors:ladybug.checkFailed) with interpolation where needed; keep
ladybugInstallCache and clearLadybugInstallCache untouched otherwise.
In `@apps/frontend/src/main/terminal/claude-integration-handler.ts`:
- Around line 180-208: The command builders mix cmd.exe and PowerShell syntax;
update buildPathPrefix and buildClaudeShellCommand to accept shellType
(propagate terminal.shellType through call sites) and branch on it: when
shellType === 'powershell' emit PowerShell forms (e.g., use $env:PATH=...; use
';' separators, use & '<cmd>' to invoke, and Remove-Item for deletions) and when
not Windows or cmd.exe keep existing cmd.exe forms (set "PATH=..." &&, call,
del, &&); also update generateTokenTempFileContent to produce
PowerShell-compatible temp-file content when shellType === 'powershell' and
ensure escapeShellCommand is used consistently with the passed shellType.
In `@apps/frontend/src/renderer/components/context/MemoriesTab.tsx`:
- Around line 162-164: Replace hardcoded user strings in the MemoriesTab
component by importing and using useTranslation and translation keys: call const
{ t } = useTranslation() and change InfoItem label props ("Database", "Path")
and fallback values (memoriesDir fallback 'memories directory' and
memoryStatus.database fallback 'auto_claude_memory') to t('memory.database'),
t('memory.path'), t('memory.fallbacks.auto_claude_memory'), and
t('memory.fallbacks.memories_directory') (or similar keys you add). Also replace
the error string "Graphiti memory is not configured" with
t('memory.errors.graphiti_not_configured'). Add those keys to your translation
JSON (e.g., common.json or memory.json) for all supported locales. Ensure
InfoItem and any render branches reference the translated strings via t(...)
instead of hardcoded text.
In `@apps/frontend/src/renderer/components/onboarding/GraphitiStep.tsx`:
- Around line 828-859: Replace the three hardcoded strings in the LadybugDB
status block inside the GraphitiStep component (the JSX guarded by
ladybugInstalled === false && ladybugError) with i18n translation keys using the
react-i18next t function: import and use the useTranslation hook (e.g., const {
t } = useTranslation()) and change "LadybugDB Not Installed", "Download Visual
Studio Build Tools", and "After installing build tools, restart the application
to retry." to t('onboarding.ladybug.notInstalled'),
t('onboarding.ladybug.downloadBuildTools'), and
t('onboarding.ladybug.restartAfterInstall') (or your project's key naming
convention), respectively; keep the conditional that shows the external link
when ladybugError.includes('Visual Studio Build Tools') intact so only the
visible strings are replaced with translation keys.
- Around line 176-181: The catch block that currently only sets
setKuzuAvailable(false) and setLadybugInstalled(false) should also set a generic
error message via setLadybugError(...) so the UI warning card (which checks
ladybugInstalled === false && ladybugError) displays on failures of
getMemoryInfrastructureStatus; update the catch in the infra-check logic (where
getMemoryInfrastructureStatus is called and setKuzuAvailable/setLadybugInstalled
are used) to call setLadybugError with a concise user-facing message and ensure
setLadybugInstalled(false) remains set, leaving setIsCheckingInfra(false) in
finally.
In `@apps/frontend/src/renderer/components/terminal/useXterm.ts`:
- Around line 49-65: Extract the DPI-adjusted font size logic into a single
helper function (e.g., getAdjustedFontSize(baseFontSize = 13)) and replace the
duplicated calculations in both the XTerm initialization (where adjustedFontSize
is currently computed before new XTerm({... fontSize:
Math.round(adjustedFontSize) ...})) and the handleDPIChange handler (the
duplicate sqrt-based scaling and min 11px logic) to call this helper and use
Math.round on its result; ensure the helper uses window.devicePixelRatio || 1,
applies 1 / Math.sqrt(scaleFactor) when scaleFactor > 1, enforces a minimum of
11, and is imported/defined in the same module so both initialization and
handleDPIChange call the single function.
apps/frontend/src/renderer/components/onboarding/GraphitiStep.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/renderer/components/onboarding/GraphitiStep.tsx (1)
865-879: Localize the newly added LadybugDB status messages.The four user-facing strings added in this component (lines 865-879 and 884-896) should use i18n translation keys to comply with frontend localization standards. The file already uses i18n keys elsewhere (e.g.,
t('onboarding:ladybug.notInstalled')), so add the new keys to the onboarding namespace and update the component to use them.Suggested i18n keys to add
"ladybug": { "notInstalled": "LadybugDB Not Installed", "downloadBuildTools": "Download Visual Studio Build Tools", "restartAfterInstall": "After installing build tools, restart the application to retry.", "checkFailed": "Failed to check infrastructure status", "dbCreateTitle": "Database will be created automatically", "dbCreateBody": "LadybugDB uses an embedded database - no Docker required. The database will be created when you first use memory features.", "readyTitle": "LadybugDB Ready", "readyBody": "Memory database is installed and available." }Then update the component to use
t('onboarding:ladybug.dbCreateTitle'),t('onboarding:ladybug.dbCreateBody'),t('onboarding:ladybug.readyTitle'), andt('onboarding:ladybug.readyBody').
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/memory-service.ts`:
- Around line 770-828: In checkLadybugInstalled, after calling spawnSync (the
variable result), add an explicit check for result.error and handle spawn
failures before inspecting result.status/stdout: if result.error.code ===
'ENOENT' set ladybugInstallCache to installed: false, pythonAvailable: false and
error: LADYBUG_ERROR_KEYS.pythonNotFound; for other errors (including timeouts)
set installed: false, pythonAvailable: true and error:
LADYBUG_ERROR_KEYS.checkFailed (or a new error key if preferred); ensure this
check runs before the existing result.status === 0 branch so missing executables
and spawn failures aren’t misclassified as “not installed.” Use the existing
symbols (spawnSync result, LADYBUG_ERROR_KEYS, ladybugInstallCache,
parsePythonCommand, getMemoryPythonEnv) to locate and update the logic.
In `@apps/frontend/src/renderer/components/onboarding/GraphitiStep.tsx`:
- Around line 146-166: The useEffect that calls
window.electronAPI.getMemoriesDir can call setMemoriesDir after the component
unmounts; add a mounted flag inside that useEffect (e.g., let isMounted = true;
and a cleanup that sets it false) and only call setMemoriesDir or
setMemoriesDir-related state when isMounted is true; keep the existing
result.success/data checks and ensure the .catch handler also checks isMounted
before touching state or logging if needed.
- Around line 996-998: Replace the hardcoded fallback text for the memories
directory in the GraphitiStep component: change the paragraph rendering that
uses memoriesDir || 'memories directory' to use i18n keys and interpolation (use
the t function with keys onboarding:memory.storedIn and
onboarding:memory.defaultDir, passing memoriesDir as the interpolated value).
Also add the new keys onboarding:memory.storedIn and
onboarding:memory.defaultDir to the en/onboarding.json and fr/onboarding.json
translation files so the component can render a localized message when
memoriesDir is absent.
- Around line 173-181: The code currently reads memory fields without verifying
result.success, causing result.error to be ignored; update the block that calls
window.electronAPI.getMemoryInfrastructureStatus() so you first check
result?.success before accessing result.data: only extract memory and call
setKuzuAvailable/setLadybugInstalled/setLadybugError from memory when
result.success is true, otherwise setKuzuAvailable(false),
setLadybugInstalled(false) and setLadybugError(result?.error ??
t('onboarding:ladybug.checkFailed')); ensure setLadybugError is also populated
in the existing catch block for thrown exceptions.
In `@apps/frontend/src/renderer/components/terminal/useXterm.ts`:
- Around line 383-454: The debounced handlers (created for handleResize and
handleDPIChange inside the useEffect) should be cancelled on cleanup to avoid
pending invocations after unmount; capture the debounced functions in variables
(e.g., handleResize and handleDPIChange as returned by debounce), and in the
effect cleanup call their cancel() method (or clear the timer if using a custom
debounce) before removing listeners and tearing down currentMediaQuery; ensure
registerDPIListener and currentMediaQuery logic still removes listeners as-is
and that you cancel the debounce for both the test fallback resize handler and
the DPI-change handler created in this effect.
In `@apps/frontend/src/shared/i18n/locales/en/memory.json`:
- Around line 1-10: These translation keys in memory.json must be moved into
approved namespaces: relocate "database", "path" and the "fallbacks" entries
("auto_claude_memory", "memories_directory") into settings.json under a new
"memory" section (e.g., settings:memory.database, settings:memory.path,
settings:memory.fallbacks.auto_claude_memory,
settings:memory.fallbacks.memories_directory), move the error
"graphiti_not_configured" into errors.json (errors:graphiti_not_configured),
update any code that imports or references the old keys to use the new
namespaced keys, and remove the unused memory.json file.
apps/frontend/src/renderer/components/onboarding/GraphitiStep.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/frontend/src/renderer/components/project-settings/SecuritySettings.tsx (1)
444-459: Add i18n translation keys for the fallback text.The hardcoded
'memories directory'fallback strings are new user-facing text introduced in this PR and should use i18n translation keys per the coding guidelines. SinceMemoriesTab.tsxalready usest('memory:fallbacks.memories_directory')for the same purpose, apply the same pattern here.🔧 Suggested fix
Add
useTranslationimport and use translation keys:+ import { useTranslation } from 'react-i18next'; import { useMemoriesDir } from '../../hooks/useMemoriesDir'; // ... in component: + const { t } = useTranslation(['memory']); const memoriesDir = useMemoriesDir();Then update the strings:
<p className="text-xs text-muted-foreground"> - Stored in {memoriesDir || 'memories directory'} + Stored in {memoriesDir || t('memory:fallbacks.memories_directory')} </p> // ... <p className="text-xs text-muted-foreground"> - Custom storage location. Default: {memoriesDir || 'memories directory'} + Custom storage location. Default: {memoriesDir || t('memory:fallbacks.memories_directory')} </p> <Input - placeholder={memoriesDir || 'memories directory'} + placeholder={memoriesDir || t('memory:fallbacks.memories_directory')}As per coding guidelines, all user-facing text in the frontend must use i18n translation keys.
apps/frontend/src/renderer/components/project-settings/MemoryBackendSection.tsx (1)
485-500: Add i18n translation keys for the fallback text.Similar to
SecuritySettings.tsx, the hardcoded'memories directory'fallback strings should use i18n translation keys for consistency withMemoriesTab.tsxwhich usest('memory:fallbacks.memories_directory').🔧 Suggested fix
Add
useTranslationand use translation keys:+ import { useTranslation } from 'react-i18next'; // ... in component: + const { t } = useTranslation(['memory']); <p className="text-xs text-muted-foreground"> - Name for the memory database (stored in {memoriesDir || 'memories directory'}) + Name for the memory database (stored in {memoriesDir || t('memory:fallbacks.memories_directory')}) </p> // ... <p className="text-xs text-muted-foreground"> - Custom storage location. Default: {memoriesDir || 'memories directory'} + Custom storage location. Default: {memoriesDir || t('memory:fallbacks.memories_directory')} </p> <Input - placeholder={memoriesDir || 'memories directory'} + placeholder={memoriesDir || t('memory:fallbacks.memories_directory')}As per coding guidelines, all user-facing text must use i18n translation keys.
🤖 Fix all issues with AI agents
In `@apps/frontend/src/renderer/hooks/useMemoriesDir.ts`:
- Around line 10-20: The effect in useMemoriesDir can call setMemoriesDir after
unmount; add a mounted flag inside the useEffect (e.g., let mounted = true) and
in the promise handlers check mounted before calling setMemoriesDir or logging
errors, then set mounted = false in the effect cleanup so no state updates occur
after unmount; reference useEffect, window.electronAPI.getMemoriesDir, and
setMemoriesDir when locating where to add the mounted guard and cleanup.
♻️ Duplicate comments (1)
apps/frontend/src/renderer/components/onboarding/GraphitiStep.tsx (1)
986-988: Localize the memories directory fallback text with i18n.This hardcoded string was flagged in a past review and should use translation keys for consistency with other components.
🔧 Suggested fix
<p className="text-xs text-muted-foreground"> - Stored in {memoriesDir || 'memories directory'} + {t('onboarding:memory.storedIn', { + path: memoriesDir || t('onboarding:memory.defaultDir') + })} </p>Add corresponding keys to translation files:
// en/onboarding.json { "memory": { "storedIn": "Stored in {{path}}", "defaultDir": "memories directory" } }As per coding guidelines, all user-facing text must use i18n translation keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/renderer/components/onboarding/GraphitiStep.tsx (1)
854-887: Use i18n for new LadybugDB status strings.The "Database will be created automatically", "LadybugDB uses an embedded database...", "LadybugDB Ready", and "Memory database is installed and available." strings are user-facing text that must use i18n keys per coding guidelines.
Add these keys to
onboarding.json(en/fr) and reference them viat()in GraphitiStep.tsx. The onboarding namespace is already established and the file already usesuseTranslation.🌐 Suggested change
- <p className="text-sm font-medium text-info"> - Database will be created automatically - </p> - <p className="text-sm text-info/80 mt-1"> - LadybugDB uses an embedded database - no Docker required. - The database will be created when you first use memory features. - </p> + <p className="text-sm font-medium text-info"> + {t('onboarding:ladybug.dbAutoCreateTitle')} + </p> + <p className="text-sm text-info/80 mt-1"> + {t('onboarding:ladybug.dbAutoCreateBody')} + </p> ... - <p className="text-sm font-medium text-success"> - LadybugDB Ready - </p> - <p className="text-sm text-success/80 mt-1"> - Memory database is installed and available. - </p> + <p className="text-sm font-medium text-success"> + {t('onboarding:ladybug.readyTitle')} + </p> + <p className="text-sm text-success/80 mt-1"> + {t('onboarding:ladybug.readyBody')} + </p>
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/memory-service.ts`:
- Around line 770-844: checkLadybugInstalled currently does a blocking spawnSync
(with timeout) and is invoked synchronously from getMemoryServiceStatus via the
MEMORY_STATUS IPC handler, which can freeze the UI on first call; change the
flow so the heavyweight Python check runs asynchronously off the IPC critical
path: convert checkLadybugInstalled to an async function that uses
child_process.spawn (or spawn wrapped in a Promise) instead of spawnSync, update
getMemoryServiceStatus to call the async check non-blockingly (await only when
safe or return a cached/initial "checking" status and kick off the async check
to populate ladybugInstallCache), and ensure error/caching logic in
checkLadybugInstalled (and the exported symbol used by IPC) remains identical
(keep LADYBUG_ERROR_KEYS handling and parsing of stdout/stderr) so callers can
rely on the same cache shape.
♻️ Duplicate comments (1)
apps/frontend/src/renderer/components/onboarding/GraphitiStep.tsx (1)
160-167: Handle unsuccessful IPC results before reading memory fields.If the IPC call returns
success: falsewithout throwing,ladybugInstalled/ladybugErrorremain null and the warning card never shows. This looks like a regression of a previously addressed bug.🐛 Proposed fix
try { const result = await window.electronAPI.getMemoryInfrastructureStatus(); const memory = result?.data?.memory; - setKuzuAvailable(result?.success && memory?.kuzuInstalled ? true : false); - setLadybugInstalled(memory?.ladybugInstalled ?? null); - setLadybugError(memory?.ladybugError ?? null); + if (!result?.success) { + setKuzuAvailable(false); + setLadybugInstalled(false); + setLadybugError(result?.error ?? t('onboarding:ladybug.checkFailed')); + return; + } + setKuzuAvailable(memory?.kuzuInstalled ?? false); + setLadybugInstalled(memory?.ladybugInstalled ?? null); + setLadybugError(memory?.ladybugError ?? null); } catch (err) {
|
@AndyMik90 All CodeRabbit review comments have been addressed: Latest commits:
Changes summary:
Ready for re-review when you have time! 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/ipc-handlers/memory-handlers.ts`:
- Around line 416-428: The handler registered on ipcMain.handle for
IPC_CHANNELS.MEMORY_GET_DIR (the async function that calls getDefaultDbPath)
returns a hardcoded user-facing string on error; change that to return the i18n
translation key (e.g., "errors:memory.getDirFailed") instead of 'Failed to get
memories directory' so the renderer can localize messages—update the error
return in the catch block to use the translation key while preserving the
existing Error type check (error instanceof Error ? error.message :
'errors:memory.getDirFailed') or otherwise ensure the returned error field is
the i18n key.
In `@apps/frontend/src/main/memory-service.ts`:
- Line 615: Replace the hardcoded user-facing string assigned to message
(currently using path.join(data.databasePath, data.database)) with an i18n
translation key; call the app's i18n translate function to produce the message
using the key "errors:memory.databaseNotFound" and pass the interpolated path
(path.join(data.databasePath, data.database)) as a parameter so the message
becomes i18n.t('errors:memory.databaseNotFound', { path:
path.join(data.databasePath, data.database) }) instead of the literal string.
- Around line 891-914: The sync wrapper checkLadybugInstalled() currently
returns pythonAvailable: true while the async probe runs, which can mislead
legacy sync callers; before returning the temporary "checking" status, call the
local findPythonCommand() (or equivalent helper used elsewhere) and if it
indicates Python is not present, return { installed: false, pythonAvailable:
false } immediately; still kick off checkLadybugInstalledAsync() in the
background as before, and ensure you reference and use findPythonCommand(),
checkLadybugInstalled(), and checkLadybugInstalledAsync() so callers get an
accurate short‑circuit result without changing the async behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/memory-service.ts (1)
599-630: Success message at line 630 is hardcoded.The error paths correctly use i18n keys, but the success message at line 630 (
"Connected to LadybugDB with ${dbCount} databases") is a hardcoded user-facing string. For consistency with the i18n approach and coding guidelines, this should also use a translation key with interpolation.♻️ Suggested fix
Add a new key to
LADYBUG_ERROR_KEYS(or a separate success keys constant):export const LADYBUG_ERROR_KEYS = { pythonNotFound: 'errors:ladybug.pythonNotFound', notInstalled: 'errors:ladybug.notInstalled', buildTools: 'errors:ladybug.buildTools', checkFailed: 'errors:ladybug.checkFailed', databaseNotFound: 'errors:ladybug.databaseNotFound', statusCheckFailed: 'errors:ladybug.statusCheckFailed', connectionFailed: 'errors:ladybug.connectionFailed', + connectionSuccess: 'memory:ladybug.connectionSuccess', } as const;Then at line 630:
- message: `Connected to LadybugDB with ${dbCount} databases`, + message: LADYBUG_ERROR_KEYS.connectionSuccess, + // Renderer should interpolate with { count: dbCount }
🤖 Fix all issues with AI agents
In `@apps/frontend/src/shared/i18n/locales/en/errors.json`:
- Line 2: The error locale key uses snake_case ("graphiti_not_configured") while
other keys use camelCase; rename the key to camelCase ("graphitiNotConfigured")
in the locales JSON and update every code reference that reads this locale key
(search for "graphiti_not_configured") to the new "graphitiNotConfigured"
identifier, ensuring consistency with existing keys like "pythonNotFound" and
"getDirFailed" and that any i18n lookup functions still resolve the renamed key.
In `@apps/frontend/src/shared/i18n/locales/en/onboarding.json`:
- Around line 111-118: The en onboarding JSON contains duplicate keys "storedIn"
and "defaultDir" (e.g., entries for "storedIn": "Stored in {{path}}" and
"defaultDir": "memories directory" appear twice); remove the duplicate entries
so each i18n key appears only once (keep the intended value for "storedIn" and
"defaultDir" and delete the redundant pair), and verify any other locale files
don't have the same duplicate to prevent silent overrides.
In `@apps/frontend/src/shared/i18n/locales/fr/errors.json`:
- Around line 19-20: Update the French translation string for the key
"memory.getDirFailed" to use the singular term consistent with the rest of the
UI (e.g., change "mémoires" to "mémoire"), ensuring the message matches the
singular phrasing used elsewhere like "mémoire Graphiti"; locate and edit the
"memory.getDirFailed" entry in the locales/fr/errors.json and replace the plural
noun so all memory-related keys use the singular form.
In `@apps/frontend/src/shared/i18n/locales/fr/onboarding.json`:
- Around line 111-118: The JSON contains duplicate keys storedIn and defaultDir
inside the memory object; remove the first occurrence of storedIn/"Stocké dans
{{path}}" and defaultDir/"répertoire de mémoires" and keep the later entries,
ensuring defaultDir uses "répertoire des mémoires" (the preferred translation)
so there are no duplicate keys and the memory object remains valid JSON.
♻️ Duplicate comments (1)
apps/frontend/src/main/memory-service.ts (1)
900-918: Sync wrapper still misreports Python availability on first call.The temporary "checking" status (lines 913-917) returns
pythonAvailable: truebefore the async check completes. If Python is actually missing, legacy callers relying on this sync function will receive incorrect information until the cache is populated.Consider short-circuiting with a
findPythonCommand()check as suggested in the previous review:♻️ Suggested fix
export function checkLadybugInstalled(): LadybugInstallStatus { // Return cached result if available if (ladybugInstallCache !== null) { return ladybugInstallCache; } + // Quick check for Python availability before async check + const pythonCmd = findPythonCommand(); + if (!pythonCmd) { + ladybugInstallCache = { + installed: false, + pythonAvailable: false, + error: LADYBUG_ERROR_KEYS.pythonNotFound, + }; + return ladybugInstallCache; + } + // Return "checking" status and kick off async check // This prevents blocking the main process checkLadybugInstalledAsync().catch(() => { // Error is handled inside the async function }); // Return a temporary status indicating check is in progress return { installed: false, pythonAvailable: true, // No error - check is still in progress }; }
c8d8151 to
e982080
Compare
- Add JSDOM guard for window.matchMedia to fix test failures
- Re-register DPI media query listener after changes (MDN pattern)
- Fix DPI scaling comment to match sqrt() implementation
- Change PowerShell escaping to use single quotes (safer, no expansion)
- Replace dynamic require('child_process') with static spawnSync import
- Add Linux test coverage for escapeShellCommand
- Update PowerShell tests to use single quote expectations
Addresses review comments from CodeRabbit, Gemini Code Assist, and maintainer.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (1)
apps/frontend/src/renderer/components/project-settings/SecuritySettings.tsx (1)
444-459: Use existing i18n key for the new fallback text ('memories directory').The newly introduced fallback string should reference the existing translation key
settings:memory.fallbacks.memories_directoryinstead of hardcoding'memories directory'. Replace the three occurrences in lines 445, 456, and 463 with:{memoriesDir || t('settings:memory.fallbacks.memories_directory')}
🤖 Fix all issues with AI agents
In
`@apps/frontend/src/renderer/components/project-settings/MemoryBackendSection.tsx`:
- Around line 497-500: The fallback string "memories directory" is hardcoded in
the MemoryBackendSection component; replace both occurrences (the JSX text:
Custom storage location. Default: {memoriesDir || 'memories directory'} and the
Input placeholder prop placeholder={memoriesDir || 'memories directory'}) to use
the i18n key t('memory.fallbacks.memories_directory') as the fallback (e.g.,
memoriesDir || t('memory.fallbacks.memories_directory')), ensuring you
import/use the existing t function in that component.
- Line 485: The JSX currently hardcodes the fallback string 'memories
directory'; update MemoryBackendSection (component) to use i18n by importing
useTranslation, calling const { t } = useTranslation() at the top of the
component, and replace the hardcoded fallback (memoriesDir || 'memories
directory') with (memoriesDir || t('memory.fallbacks.memories_directory'));
ensure any other similar fallback occurrences in MemoryBackendSection use the
same t(...) key.
♻️ Duplicate comments (2)
apps/frontend/src/renderer/components/onboarding/GraphitiStep.tsx (1)
163-174: Check result.success before extracting memory data.When
result.successis false (API error without exception),result.datais undefined, somemorywill be undefined and bothladybugInstalledandladybugErrorwill be set tonull. This means the warning card (line 822) won't display since it requiresladybugErrorto be truthy.🐛 Suggested fix to handle API errors
try { const result = await window.electronAPI.getMemoryInfrastructureStatus(); + if (!result?.success) { + setKuzuAvailable(false); + setLadybugInstalled(false); + setLadybugError(result?.error ?? t('onboarding:ladybug.checkFailed')); + return; + } const memory = result?.data?.memory; - setKuzuAvailable(result?.success && memory?.kuzuInstalled ? true : false); + setKuzuAvailable(memory?.kuzuInstalled ?? false); setLadybugInstalled(memory?.ladybugInstalled ?? null); setLadybugError(memory?.ladybugError ?? null); } catch (err) {apps/frontend/src/main/memory-service.ts (1)
900-921: Consider short-circuiting when Python is not found.The sync wrapper correctly triggers the async check and returns a temporary status. However, when
pythonAvailableis false (line 907), returning without an error message may cause callers to miss the Python-not-found condition until the async check completes.♻️ Optional: Short-circuit for missing Python
export function checkLadybugInstalled(): LadybugInstallStatus { // Return cached result if available if (ladybugInstallCache !== null) { return ladybugInstallCache; } // Quick sync check for Python availability (doesn't block, just checks PATH) const pythonAvailable = findPythonCommand() !== null; + + // Short-circuit if Python is not available + if (!pythonAvailable) { + ladybugInstallCache = { + installed: false, + pythonAvailable: false, + error: LADYBUG_ERROR_KEYS.pythonNotFound, + }; + return ladybugInstallCache; + } // Return "checking" status and kick off async check // This prevents blocking the main process checkLadybugInstalledAsync().catch(() => {
| <Label className="text-sm font-medium text-foreground">Database Name</Label> | ||
| <p className="text-xs text-muted-foreground"> | ||
| Name for the memory database (stored in ~/.auto-claude/memories/) | ||
| Name for the memory database (stored in {memoriesDir || 'memories directory'}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use i18n translation keys for fallback strings.
The hardcoded fallback 'memories directory' should use a translation key. The settings.json already defines memory.fallbacks.memories_directory for this purpose.
As per coding guidelines, all user-facing text must use i18n translation keys.
🛠️ Proposed fix
Add useTranslation import and use the i18n key:
import { useMemoriesDir } from '../../hooks/useMemoriesDir';
+import { useTranslation } from 'react-i18next';Then in the component:
+ const { t } = useTranslation('settings');
const memoriesDir = useMemoriesDir();And update the fallbacks:
- Name for the memory database (stored in {memoriesDir || 'memories directory'})
+ Name for the memory database (stored in {memoriesDir || t('memory.fallbacks.memories_directory')})🤖 Prompt for AI Agents
In
`@apps/frontend/src/renderer/components/project-settings/MemoryBackendSection.tsx`
at line 485, The JSX currently hardcodes the fallback string 'memories
directory'; update MemoryBackendSection (component) to use i18n by importing
useTranslation, calling const { t } = useTranslation() at the top of the
component, and replace the hardcoded fallback (memoriesDir || 'memories
directory') with (memoriesDir || t('memory.fallbacks.memories_directory'));
ensure any other similar fallback occurrences in MemoryBackendSection use the
same t(...) key.
| Custom storage location. Default: {memoriesDir || 'memories directory'} | ||
| </p> | ||
| <Input | ||
| placeholder="~/.auto-claude/memories" | ||
| placeholder={memoriesDir || 'memories directory'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same i18n issue: replace hardcoded fallback strings.
Lines 497 and 500 also use the hardcoded 'memories directory' fallback. Apply the same fix using t('memory.fallbacks.memories_directory').
🛠️ Proposed fix
<p className="text-xs text-muted-foreground">
- Custom storage location. Default: {memoriesDir || 'memories directory'}
+ Custom storage location. Default: {memoriesDir || t('memory.fallbacks.memories_directory')}
</p>
<Input
- placeholder={memoriesDir || 'memories directory'}
+ placeholder={memoriesDir || t('memory.fallbacks.memories_directory')}🤖 Prompt for AI Agents
In
`@apps/frontend/src/renderer/components/project-settings/MemoryBackendSection.tsx`
around lines 497 - 500, The fallback string "memories directory" is hardcoded in
the MemoryBackendSection component; replace both occurrences (the JSX text:
Custom storage location. Default: {memoriesDir || 'memories directory'} and the
Input placeholder prop placeholder={memoriesDir || 'memories directory'}) to use
the i18n key t('memory.fallbacks.memories_directory') as the fallback (e.g.,
memoriesDir || t('memory.fallbacks.memories_directory')), ensuring you
import/use the existing t function in that component.
67a743f to
e83e445
Compare
…dows The Memory settings section was displaying Unix-style paths (~/.auto-claude/memories) instead of Windows paths (C:\Users\<user>\.auto-claude\memories). This fix: - Adds new IPC handler (MEMORY_GET_DIR) to return platform-specific memories path - Updates MemoryBackendSection, SecuritySettings, MemoriesTab to fetch and display correct path - Updates GraphitiStep (onboarding wizard) to show correct platform path - Fixes path separator in memory-service.ts error message (uses path.join instead of /) - Updates env-handlers to use dynamic path in .env template - Removes trailing slashes from path displays to avoid mixed separators Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
… messages - Add checkLadybugInstalled() function to detect if real_ladybug Python package is installed - Show detailed error message in Setup Wizard when LadybugDB is not installed - Include link to Visual Studio Build Tools download for Windows users - Update InfrastructureStatus component with same error handling - Add ladybugInstalled and ladybugError fields to MemoryServiceStatus interface On Windows, LadybugDB requires Visual Studio Build Tools to compile. This change provides clear guidance to users when installation fails. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PowerShell interprets -- flags as the pre-decrement operator, causing errors like "The '--' operator works only on variables or on properties". Changes: - Add shellType parameter to escapeShellCommand function - Use call operator (&) prefix for PowerShell commands: & "cmd.exe" - Update all 4 escapeShellCommand calls to pass terminal.shellType - Add comprehensive tests for escapeShellCommand function This fixes Claude CLI invocation when using PowerShell as the terminal. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add JSDOM guard for window.matchMedia to fix test failures
- Re-register DPI media query listener after changes (MDN pattern)
- Fix DPI scaling comment to match sqrt() implementation
- Change PowerShell escaping to use single quotes (safer, no expansion)
- Replace dynamic require('child_process') with static spawnSync import
- Add Linux test coverage for escapeShellCommand
- Update PowerShell tests to use single quote expectations
Addresses review comments from CodeRabbit, Gemini Code Assist, and maintainer.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…e i18n Changes: - useXterm.ts: Extract getAdjustedFontSize() helper to reduce duplication - claude-integration-handler.ts: Add PowerShell support for Windows terminals - buildPathPrefix() now uses $env:PATH syntax for PowerShell - buildClaudeShellCommand() supports PowerShell command separators and escaping - generateTokenTempFileContent() generates .ps1 files for PowerShell - getTempFileExtension() returns .ps1 for PowerShell, .bat for cmd.exe - memory-service.ts: Add env to checkLadybugInstalled() spawnSync call - Return i18n error keys instead of hardcoded messages - Export LADYBUG_ERROR_KEYS for frontend translation - MemoriesTab.tsx: Replace hardcoded strings with i18n translations - GraphitiStep.tsx: Use i18n for LadybugDB status messages and errors - Add translation keys to errors.json and onboarding.json (en/fr) - Create memory.json namespace for memory-related translations Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ndler
CodeRabbit nitpick fix:
- Extract duplicated memoriesDir fetching logic into useMemoriesDir hook
- Update SecuritySettings, MemoryBackendSection, MemoriesTab, and GraphitiStep
to use the new hook instead of duplicated useEffect logic
Gemini Code Assist bug fix:
- Simplify DPI change handler in useXterm.ts using useState for dpr
- Use { once: true } pattern for matchMedia listener that auto-re-registers
when dpr state changes, fixing the bug where only first DPI change was detected
- Remove complex registerDPIListener() logic in favor of simpler state-based approach
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- useMemoriesDir: add isMounted guard to prevent state updates after unmount - Move memory.json keys to approved namespaces (settings.json and errors.json) - Remove deprecated memory namespace from i18n configuration - useXterm: add debounce cancellation support for DPI and resize handlers Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- memory-service.ts: add result.error check before result.status to handle ENOENT (missing Python) and timeout scenarios - GraphitiStep.tsx: localize "memories directory" fallback using i18n - Add storedIn and defaultDir keys to en/onboarding.json - Add French translations to fr/onboarding.json Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace blocking spawnSync with async spawn wrapped in Promise - Add checkLadybugInstalledAsync() as the new primary function - Add getMemoryServiceStatusAsync() for async IPC handlers - Update MEMORY_STATUS and MEMORY_LIST_DATABASES handlers to use async - Preserve backward compatibility with deprecated sync versions - Add deduplication to prevent concurrent redundant checks Addresses CodeRabbit review comment about blocking IPC calls. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ogic - Replace hardcoded error strings with i18n keys in memory-handlers.ts - Use i18n keys for database status messages in memory-service.ts - Fix pythonAvailable incorrectly set to true in catch block - Add new i18n keys: databaseNotFound, statusCheckFailed, connectionFailed - Add memory.storedIn and memory.defaultDir keys for GraphitiStep Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ng status The sync wrapper now performs a quick check for Python availability using findPythonCommand() before returning the temporary status. This ensures pythonAvailable accurately reflects whether Python is found, even on the first call before the async check completes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove duplicate storedIn/defaultDir keys from onboarding.json - Change graphiti_not_configured to camelCase for consistency - Use singular "mémoire" terminology in French translations Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Added -ErrorAction Stop to the Remove-Item command in PowerShell integration to ensure that if the temp file deletion fails, execution stops immediately. This prevents the sensitive OAuth token file from being silently left on disk, matching the safer behavior of cmd.exe which uses && to abort on failure. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
e982080 to
1ec7237
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (3)
apps/frontend/src/renderer/lib/mocks/infrastructure-mock.ts (1)
6-39: Optional: de-duplicate the memories path constant in the mock.The same path literal is now used in multiple places; centralizing it avoids future drift.
♻️ Suggested refactor
+const MEMORY_DIR = '~/.auto-claude/memories'; + export const infrastructureMock = { // Memory Infrastructure Operations (LadybugDB) getMemoryInfrastructureStatus: async () => ({ success: true, data: { memory: { kuzuInstalled: true, ladybugInstalled: true, - databasePath: '~/.auto-claude/memories', + databasePath: MEMORY_DIR, databaseExists: true, databases: ['auto_claude_memory'] }, ready: true } }), @@ getMemoriesDir: async () => ({ success: true, - data: '~/.auto-claude/memories' + data: MEMORY_DIR }),apps/frontend/src/renderer/components/project-settings/SecuritySettings.tsx (1)
442-460: Localize the new memory-path descriptions and fallback text.The new user-facing strings on lines 444, 455, and 456 ("Stored in", "Custom storage location. Default:") and the hardcoded fallback "memories directory" must use i18n keys. The fallback key
settings:memory.fallbacks.memories_directoryalready exists in the locale files, but you need to add the descriptive text as new i18n keys.Add these keys to both
apps/frontend/src/shared/i18n/locales/en/settings.jsonandapps/frontend/src/shared/i18n/locales/fr/settings.jsonunder thememorysection:{ "databaseDescription": "Stored in {path}", "pathDescription": "Custom storage location. Default: {path}" }Then update the code to use interpolation:
Suggested implementation
- <p className="text-xs text-muted-foreground"> - Stored in {memoriesDir || 'memories directory'} - </p> + <p className="text-xs text-muted-foreground"> + {t('settings:memory.databaseDescription', { + path: memoriesDir || t('settings:memory.fallbacks.memories_directory') + })} + </p> ... - <p className="text-xs text-muted-foreground"> - Custom storage location. Default: {memoriesDir || 'memories directory'} - </p> + <p className="text-xs text-muted-foreground"> + {t('settings:memory.pathDescription', { + path: memoriesDir || t('settings:memory.fallbacks.memories_directory') + })} + </p> - <Input - placeholder={memoriesDir || 'memories directory'} + <Input + placeholder={memoriesDir || t('settings:memory.fallbacks.memories_directory')}apps/frontend/src/renderer/components/onboarding/GraphitiStep.tsx (1)
855-867: Localize hardcoded user-facing strings to follow i18n requirements.The file already uses i18n keys elsewhere (e.g.,
t('onboarding:ladybug.notInstalled')), but lines 862–867 and 883–886 contain hardcoded strings that should use translation keys:
- "Database will be created automatically"
- "LadybugDB uses an embedded database - no Docker required. The database will be created when you first use memory features."
- "LadybugDB Ready"
- "Memory database is installed and available."
Add i18n keys to
onboarding.jsonlocale files and replace these strings witht()calls to maintain consistency with the rest of the component and comply with coding guidelines.
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/memory-service.ts`:
- Around line 793-810: Replace calls to findPythonCommand() with
getBackendPythonPath() when performing the real_ladybug import check so the
install probe uses the same backend venv resolver as memory queries; remove the
now-unnecessary null check around python command existence in the ladybug
install flow (the block that sets ladybugInstallCache when pythonCmd is falsy)
and always proceed to parsePythonCommand(getBackendPythonPath()) inside the
ladybugCheckInProgress promise (the area using parsePythonCommand, checkArgs and
pythonEnv/getMemoryPythonEnv()). Also update the status-reporting locations that
set pythonAvailable (previously using findPythonCommand()) to use
getBackendPythonPath() at the other two spots mentioned (the code around where
pythonAvailable is computed at the later status checks).
In
`@apps/frontend/src/renderer/components/project-settings/InfrastructureStatus.tsx`:
- Around line 29-63: InfrastructureStatus currently renders hardcoded
user-facing strings; update the component to import and call
useTranslation('settings') and replace all hardcoded labels (e.g., "LadybugDB",
"Installed", "Not Installed", "Download Visual Studio Build Tools", "Ready",
"Will be created on first use", "Requires LadybugDB", "Graph memory is ready to
use", "Graph memory requires Python 3.12+ with LadybugDB. No Docker needed.",
"Memory Infrastructure") with t('settings:memory.<key>') using the keys provided
in the review (memory.ladybug, memory.installed, memory.notInstalled,
memory.downloadBuildTools, memory.ready, memory.willBeCreatedOnFirstUse,
memory.requiresLadybug, memory.graphReadyToUse, memory.graphRequirement,
memory.infrastructureTitle) while preserving conditional logic around
ladybugInstalled and ladybugError and the external link; then add the same keys
under the memory section in the settings.json locale file so translations
resolve.
♻️ Duplicate comments (2)
apps/frontend/src/renderer/components/project-settings/MemoryBackendSection.tsx (1)
485-485: Use i18n translation keys for the fallback strings.The hardcoded fallback
'memories directory'should use a translation key. As per coding guidelines, all user-facing text must use i18n translation keys. Thesettings.jsonalready definesmemory.fallbacks.memories_directoryfor this purpose.🛠️ Proposed fix
First, add the
useTranslationimport and call in the component:import { useMemoriesDir } from '../../hooks/useMemoriesDir'; +import { useTranslation } from 'react-i18next';Inside the component:
+ const { t } = useTranslation('settings'); const memoriesDir = useMemoriesDir();Then update the fallback strings:
- Name for the memory database (stored in {memoriesDir || 'memories directory'}) + Name for the memory database (stored in {memoriesDir || t('memory.fallbacks.memories_directory')})- Custom storage location. Default: {memoriesDir || 'memories directory'} + Custom storage location. Default: {memoriesDir || t('memory.fallbacks.memories_directory')} </p> <Input - placeholder={memoriesDir || 'memories directory'} + placeholder={memoriesDir || t('memory.fallbacks.memories_directory')}Also applies to: 497-500
apps/frontend/src/renderer/components/onboarding/GraphitiStep.tsx (1)
163-167: Guard unsuccessful IPC results before reading memory fields.
Line 163: ifgetMemoryInfrastructureStatus()returnssuccess: falsewithout throwing,ladybugInstalled/ladybugErrorstaynull, so the warning card never renders. Gate onresult.successand surfaceresult.errorfirst.🐛 Proposed fix
- const result = await window.electronAPI.getMemoryInfrastructureStatus(); - const memory = result?.data?.memory; - setKuzuAvailable(result?.success && memory?.kuzuInstalled ? true : false); - setLadybugInstalled(memory?.ladybugInstalled ?? null); - setLadybugError(memory?.ladybugError ?? null); + const result = await window.electronAPI.getMemoryInfrastructureStatus(); + if (!result?.success) { + setKuzuAvailable(false); + setLadybugInstalled(false); + setLadybugError(result?.error ?? t('onboarding:ladybug.checkFailed')); + return; + } + const memory = result?.data?.memory; + setKuzuAvailable(!!memory?.kuzuInstalled); + setLadybugInstalled(memory?.ladybugInstalled ?? null); + setLadybugError(memory?.ladybugError ?? null);Also applies to: 170-171
| const pythonCmd = findPythonCommand(); | ||
| if (!pythonCmd) { | ||
| ladybugInstallCache = { | ||
| installed: false, | ||
| pythonAvailable: false, | ||
| error: LADYBUG_ERROR_KEYS.pythonNotFound, | ||
| }; | ||
| return ladybugInstallCache; | ||
| } | ||
|
|
||
| // Start the async check and track the promise | ||
| ladybugCheckInProgress = new Promise<LadybugInstallStatus>((resolve) => { | ||
| const [cmd, args] = parsePythonCommand(pythonCmd); | ||
| const checkArgs = [...args, '-c', 'import real_ladybug; print("OK")']; | ||
|
|
||
| // Use getMemoryPythonEnv() to ensure real_ladybug can be found | ||
| const pythonEnv = getMemoryPythonEnv(); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the definitions of the two functions mentioned
echo "=== Searching for findPythonCommand ==="
rg -n "findPythonCommand" apps/frontend/src/main -S -A 3
echo ""
echo "=== Searching for getBackendPythonPath ==="
rg -n "getBackendPythonPath|getConfiguredPythonPath" apps/frontend/src/main -S -A 3Repository: AndyMik90/Auto-Claude
Length of output: 17230
🏁 Script executed:
#!/bin/bash
# Get the file structure to understand the layout
ls -la apps/frontend/src/main/Repository: AndyMik90/Auto-Claude
Length of output: 3211
🏁 Script executed:
#!/bin/bash
# Check the memory-service.ts file around the specified lines
wc -l apps/frontend/src/main/memory-service.tsRepository: AndyMik90/Auto-Claude
Length of output: 109
🏁 Script executed:
#!/bin/bash
# Read the implementations of the three functions
echo "=== findPythonCommand in python-detector.ts (lines 54-100) ==="
sed -n '54,100p' apps/frontend/src/main/python-detector.ts
echo ""
echo "=== getBackendPythonPath in memory-service.ts (lines 127-158) ==="
sed -n '127,158p' apps/frontend/src/main/memory-service.ts
echo ""
echo "=== getConfiguredPythonPath in python-env-manager.ts (lines 830-841) ==="
sed -n '830,841p' apps/frontend/src/main/python-env-manager.tsRepository: AndyMik90/Auto-Claude
Length of output: 3535
🏁 Script executed:
#!/bin/bash
# Read the specific lines mentioned in the review
echo "=== Lines 793-810 in memory-service.ts ==="
sed -n '793,810p' apps/frontend/src/main/memory-service.ts
echo ""
echo "=== Lines 953-962 in memory-service.ts ==="
sed -n '953,962p' apps/frontend/src/main/memory-service.ts
echo ""
echo "=== Context around line 907 ==="
sed -n '900,920p' apps/frontend/src/main/memory-service.tsRepository: AndyMik90/Auto-Claude
Length of output: 1914
🏁 Script executed:
#!/bin/bash
# Verify the key difference: check what pythonEnvManager does
echo "=== pythonEnvManager in python-env-manager.ts ==="
rg -n "class PythonEnvManager" apps/frontend/src/main/python-env-manager.ts -A 20 | head -40
echo ""
echo "=== isEnvReady and getPythonPath methods ==="
rg -n "isEnvReady\(\)|getPythonPath\(\)" apps/frontend/src/main/python-env-manager.ts -B 2 -A 5 | head -50Repository: AndyMik90/Auto-Claude
Length of output: 1954
🏁 Script executed:
#!/bin/bash
# Check where memory queries actually run and what Python they use
echo "=== queryMemories function (uses getBackendPythonPath) ==="
sed -n '190,210p' apps/frontend/src/main/memory-service.ts
echo ""
echo "=== checkLadybugInstalledAsync context ==="
sed -n '775,830p' apps/frontend/src/main/memory-service.ts | head -50Repository: AndyMik90/Auto-Claude
Length of output: 2442
🏁 Script executed:
#!/bin/bash
# Check if getBackendPythonPath always returns a string (never null)
echo "=== Full getBackendPythonPath and getConfiguredPythonPath implementations ==="
sed -n '127,158p' apps/frontend/src/main/memory-service.ts
echo ""
sed -n '830,841p' apps/frontend/src/main/python-env-manager.ts
echo ""
echo "=== Check what happens at line 954 in context ==="
sed -n '950,970p' apps/frontend/src/main/memory-service.tsRepository: AndyMik90/Auto-Claude
Length of output: 2477
🏁 Script executed:
#!/bin/bash
# Verify: does getBackendPythonPath return a string or can be null?
echo "=== Return type of getBackendPythonPath ==="
sed -n '125,130p' apps/frontend/src/main/memory-service.ts
echo ""
echo "=== Return type of getConfiguredPythonPath ==="
sed -n '828,835p' apps/frontend/src/main/python-env-manager.tsRepository: AndyMik90/Auto-Claude
Length of output: 760
🏁 Script executed:
#!/bin/bash
# Check the logic flow one more time - in dev mode, what does each function find?
echo "=== In dev mode, getBackendPythonPath looks for backend venv ==="
sed -n '140,157p' apps/frontend/src/main/memory-service.ts
echo ""
echo "=== findPythonCommand checks bundled first, then system ==="
sed -n '54,90p' apps/frontend/src/main/python-detector.tsRepository: AndyMik90/Auto-Claude
Length of output: 2116
🏁 Script executed:
#!/bin/bash
# Verify: the issue is that in dev mode, the two functions resolve to DIFFERENT Python interpreters
# Let's confirm the backend venv is supposed to have real_ladybug
echo "=== Check backend setup in the codebase ==="
rg -n "real_ladybug" apps/frontend/src/main -A 2 -B 2 | head -50Repository: AndyMik90/Auto-Claude
Length of output: 3877
🏁 Script executed:
#!/bin/bash
# Final verification: confirm the different resolution paths
echo "=== Summary of resolution differences ==="
echo "Development mode:"
echo "1. getBackendPythonPath() -> looks for backend/.venv/bin/python (has real_ladybug)"
echo "2. findPythonCommand() -> checks bundled/system Python (may NOT have real_ladybug)"
echo ""
echo "This means in dev mode, the install check (findPythonCommand) could fail"
echo "while memory queries actually work (using getBackendPythonPath with real_ladybug)"
echo ""
echo "=== Verify: does the install check try to import real_ladybug? ==="
sed -n '793,830p' apps/frontend/src/main/memory-service.ts | grep -A 5 "import real_ladybug"Repository: AndyMik90/Auto-Claude
Length of output: 744
🏁 Script executed:
#!/bin/bash
# Check if there's any note about needing to align these
echo "=== Search for any comments about this in memory-service ==="
rg -n "findPythonCommand|getBackendPythonPath" apps/frontend/src/main/memory-service.ts -B 3 | grep -E "(findPythonCommand|getBackendPythonPath|real_ladybug)" | head -30Repository: AndyMik90/Auto-Claude
Length of output: 1214
🏁 Script executed:
#!/bin/bash
# Verify one last detail: what happens with the null check
# getBackendPythonPath always returns string, so the null check at line 794 would never trigger
echo "=== Check if getBackendPythonPath can ever return null or empty ==="
sed -n '127,158p' apps/frontend/src/main/memory-service.ts | grep -E "return|null|undefined"Repository: AndyMik90/Auto-Claude
Length of output: 209
Use getBackendPythonPath() for the install check to match memory query resolution.
In development mode, findPythonCommand() (line 793) may resolve to system Python without real_ladybug installed, while memory queries correctly use getBackendPythonPath() to find the backend venv that includes the package. This causes the install check to fail even when memory queries work (false negative). Since the check attempts to import real_ladybug, it must use the same resolver as the actual queries.
Replace findPythonCommand() with getBackendPythonPath() at line 793. Note that getBackendPythonPath() always returns a string and never null, so the null check at lines 794–800 can be removed.
Also apply the same fix at lines 954 and 982 where pythonAvailable uses findPythonCommand() for status reporting.
🤖 Prompt for AI Agents
In `@apps/frontend/src/main/memory-service.ts` around lines 793 - 810, Replace
calls to findPythonCommand() with getBackendPythonPath() when performing the
real_ladybug import check so the install probe uses the same backend venv
resolver as memory queries; remove the now-unnecessary null check around python
command existence in the ladybug install flow (the block that sets
ladybugInstallCache when pythonCmd is falsy) and always proceed to
parsePythonCommand(getBackendPythonPath()) inside the ladybugCheckInProgress
promise (the area using parsePythonCommand, checkArgs and
pythonEnv/getMemoryPythonEnv()). Also update the status-reporting locations that
set pythonAvailable (previously using findPythonCommand()) to use
getBackendPythonPath() at the other two spots mentioned (the code around where
pythonAvailable is computed at the later status checks).
| {/* LadybugDB Installation Status */} | ||
| <div className="flex items-center justify-between"> | ||
| <div className="flex items-center gap-2"> | ||
| {infrastructureStatus?.memory.kuzuInstalled ? ( | ||
| {ladybugInstalled ? ( | ||
| <CheckCircle2 className="h-4 w-4 text-success" /> | ||
| ) : ( | ||
| <AlertCircle className="h-4 w-4 text-warning" /> | ||
| )} | ||
| <span className="text-xs text-foreground">Kuzu Database</span> | ||
| <span className="text-xs text-foreground">LadybugDB</span> | ||
| </div> | ||
| <div className="flex items-center gap-2"> | ||
| {infrastructureStatus?.memory.kuzuInstalled ? ( | ||
| {ladybugInstalled ? ( | ||
| <span className="text-xs text-success">Installed</span> | ||
| ) : ( | ||
| <span className="text-xs text-warning">Not Available</span> | ||
| <span className="text-xs text-warning">Not Installed</span> | ||
| )} | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* LadybugDB Error Details */} | ||
| {!ladybugInstalled && ladybugError && ( | ||
| <div className="rounded-md bg-warning/10 border border-warning/30 p-2 space-y-1"> | ||
| <p className="text-xs text-warning">{ladybugError}</p> | ||
| {ladybugError.includes('Visual Studio Build Tools') && ( | ||
| <a | ||
| href="https://visualstudio.microsoft.com/visual-cpp-build-tools/" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="text-xs text-primary hover:text-primary/80 flex items-center gap-1" | ||
| > | ||
| <ExternalLink className="h-3 w-3" /> | ||
| Download Visual Studio Build Tools | ||
| </a> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and examine its full content
fd "InfrastructureStatus.tsx" --type fRepository: AndyMik90/Auto-Claude
Length of output: 144
🏁 Script executed:
# Check the structure of the app frontend directory
git ls-files apps/frontend/src/renderer/components/project-settings/ | head -20Repository: AndyMik90/Auto-Claude
Length of output: 1597
🏁 Script executed:
# Examine the InfrastructureStatus.tsx file
cat -n apps/frontend/src/renderer/components/project-settings/InfrastructureStatus.tsxRepository: AndyMik90/Auto-Claude
Length of output: 5235
🏁 Script executed:
# Check if this file was recently added or modified in the current PR
git log --oneline -20 -- apps/frontend/src/renderer/components/project-settings/InfrastructureStatus.tsxRepository: AndyMik90/Auto-Claude
Length of output: 134
🏁 Script executed:
# Check if i18n is set up in other components in the same directory
rg "useTranslation" apps/frontend/src/renderer/components/project-settings/ -A 2Repository: AndyMik90/Auto-Claude
Length of output: 1440
🏁 Script executed:
# Check the structure of i18n locales
fd "locales" --type dRepository: AndyMik90/Auto-Claude
Length of output: 103
🏁 Script executed:
# Check what translation namespaces exist
ls -la apps/frontend/src/shared/i18n/locales/en/ 2>/dev/null | head -20Repository: AndyMik90/Auto-Claude
Length of output: 878
🏁 Script executed:
# Check the full git log to see when this file was added
git log --all --follow --oneline -- apps/frontend/src/renderer/components/project-settings/InfrastructureStatus.tsxRepository: AndyMik90/Auto-Claude
Length of output: 266
🏁 Script executed:
# Check if there are similar strings already translated in settings.json
cat apps/frontend/src/shared/i18n/locales/en/settings.json | jq . | grep -i "installed\|ladybug\|database\|memory" -i -A 2 -B 2Repository: AndyMik90/Auto-Claude
Length of output: 1585
🏁 Script executed:
# Let's check the entire settings.json structure more carefully
head -100 apps/frontend/src/shared/i18n/locales/en/settings.jsonRepository: AndyMik90/Auto-Claude
Length of output: 2815
🏁 Script executed:
# Check if MemoryBackendSection uses i18n (since this is related to memory)
cat -n apps/frontend/src/renderer/components/project-settings/MemoryBackendSection.tsx | head -80Repository: AndyMik90/Auto-Claude
Length of output: 3733
🏁 Script executed:
# Check what changes were made to this file in current PR context
git diff HEAD -- apps/frontend/src/renderer/components/project-settings/InfrastructureStatus.tsx | head -100Repository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
# Also check git status to see current state
git status apps/frontend/src/renderer/components/project-settings/InfrastructureStatus.tsxRepository: AndyMik90/Auto-Claude
Length of output: 130
🏁 Script executed:
# Check if this file is in the current commit
git show --name-only --oneline -1 | grep InfrastructureStatusRepository: AndyMik90/Auto-Claude
Length of output: 144
🏁 Script executed:
# Let's get more context on what files are in the current PR
git log -1 --stat | head -40Repository: AndyMik90/Auto-Claude
Length of output: 2294
🏁 Script executed:
# Check the git diff for the current PR (from FETCH_HEAD)
git diff 1ec7237^..1ec7237 -- apps/frontend/src/renderer/components/project-settings/InfrastructureStatus.tsx | head -150Repository: AndyMik90/Auto-Claude
Length of output: 104
🏁 Script executed:
# Let's check if InfrastructureStatus.tsx was modified in the latest commit or is pre-existing
git show 1ec7237:apps/frontend/src/renderer/components/project-settings/InfrastructureStatus.tsx | head -80Repository: AndyMik90/Auto-Claude
Length of output: 3322
🏁 Script executed:
# Check previous commit to see if the file existed then
git show 1ec7237^:apps/frontend/src/renderer/components/project-settings/InfrastructureStatus.tsx 2>&1 | head -80Repository: AndyMik90/Auto-Claude
Length of output: 103
🏁 Script executed:
# Let's check git log more carefully to understand the timeline
git log --all --oneline --follow -- apps/frontend/src/renderer/components/project-settings/InfrastructureStatus.tsx | head -10Repository: AndyMik90/Auto-Claude
Length of output: 266
🏁 Script executed:
# Check what branch/commits we're on
git log --oneline -5Repository: AndyMik90/Auto-Claude
Length of output: 134
🏁 Script executed:
# Let's see the full commit message and files changed in the latest commit
git log -1 --name-status | head -50Repository: AndyMik90/Auto-Claude
Length of output: 2104
🏁 Script executed:
# Check if InfrastructureStatus is in the list of changed files in latest commit
git show --name-only 1ec7237 | grep "InfrastructureStatus"Repository: AndyMik90/Auto-Claude
Length of output: 144
🏁 Script executed:
# Check if the file was modified or added in the latest commit
git show --name-status 1ec7237 | grep "InfrastructureStatus"Repository: AndyMik90/Auto-Claude
Length of output: 146
🏁 Script executed:
# Compare the current version with the previous release
git diff ebe7633..1ec7237 -- apps/frontend/src/renderer/components/project-settings/InfrastructureStatus.tsxRepository: AndyMik90/Auto-Claude
Length of output: 4318
🏁 Script executed:
# If no diff, the file might not have changed in this commit. Let's check the commit before that
git show ebe7633:apps/frontend/src/renderer/components/project-settings/InfrastructureStatus.tsx 2>&1 | wc -lRepository: AndyMik90/Auto-Claude
Length of output: 67
🏁 Script executed:
# Check if there are existing translation keys for "installed", "notInstalled", "Ready", etc.
cat apps/frontend/src/shared/i18n/locales/en/settings.json | jq '.devtools'Repository: AndyMik90/Auto-Claude
Length of output: 1447
🏁 Script executed:
# Look for memory-related translation keys in settings.json
cat apps/frontend/src/shared/i18n/locales/en/settings.json | jq '.memory'Repository: AndyMik90/Auto-Claude
Length of output: 228
🏁 Script executed:
# Check how other components handle similar status indicators
rg "Installed" apps/frontend/src/renderer/components/project-settings/ -A 2 -B 2Repository: AndyMik90/Auto-Claude
Length of output: 6478
🏁 Script executed:
# Check if there's a pattern for how status strings like "Ready" are translated
rg "Ready\|ready" apps/frontend/src/shared/i18n/locales/en/ -B 2 -A 2Repository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
# Check the full memory section structure in settings.json to see what keys exist
cat apps/frontend/src/shared/i18n/locales/en/settings.json | jq '.memory'Repository: AndyMik90/Auto-Claude
Length of output: 228
Add i18n translation keys for new user-facing strings in InfrastructureStatus component.
This newly added component introduces multiple hardcoded strings (LadybugDB, Installed, Not Installed, Download Visual Studio Build Tools, Ready, Will be created on first use, Requires LadybugDB, Graph memory is ready to use, Graph memory requires Python 3.12+ with LadybugDB, Memory Infrastructure) that should use react-i18next keys per frontend guidelines.
Add useTranslation hook with 'settings' namespace and replace hardcoded strings with translation keys. For example:
- "LadybugDB" →
t('settings:memory.ladybug') - "Installed" →
t('settings:memory.installed') - "Not Installed" →
t('settings:memory.notInstalled') - "Ready" →
t('settings:memory.ready') - "Will be created on first use" →
t('settings:memory.willBeCreatedOnFirstUse') - "Requires LadybugDB" →
t('settings:memory.requiresLadybug') - "Download Visual Studio Build Tools" →
t('settings:memory.downloadBuildTools') - "Graph memory is ready to use" →
t('settings:memory.graphReadyToUse') - "Graph memory requires Python 3.12+ with LadybugDB. No Docker needed." →
t('settings:memory.graphRequirement') - "Memory Infrastructure" →
t('settings:memory.infrastructureTitle')
Then add corresponding keys to apps/frontend/src/shared/i18n/locales/en/settings.json under the memory section.
🤖 Prompt for AI Agents
In
`@apps/frontend/src/renderer/components/project-settings/InfrastructureStatus.tsx`
around lines 29 - 63, InfrastructureStatus currently renders hardcoded
user-facing strings; update the component to import and call
useTranslation('settings') and replace all hardcoded labels (e.g., "LadybugDB",
"Installed", "Not Installed", "Download Visual Studio Build Tools", "Ready",
"Will be created on first use", "Requires LadybugDB", "Graph memory is ready to
use", "Graph memory requires Python 3.12+ with LadybugDB. No Docker needed.",
"Memory Infrastructure") with t('settings:memory.<key>') using the keys provided
in the review (memory.ladybug, memory.installed, memory.notInstalled,
memory.downloadBuildTools, memory.ready, memory.willBeCreatedOnFirstUse,
memory.requiresLadybug, memory.graphReadyToUse, memory.graphRequirement,
memory.infrastructureTitle) while preserving conditional logic around
ladybugInstalled and ladybugError and the external link; then add the same keys
under the memory section in the settings.json locale file so translations
resolve.
…rm-specific memory paths
Base Branch
developbranch (required for all feature/fix PRs)main(hotfix only - maintainers)Description
This PR fixes two Windows-specific issues that significantly improve the developer experience:
Full system PATH for Bash sessions: Windows Bash sessions now have access to all installed system tools (ddev, mkcert, chocolatey, etc.) by passing the complete Windows PATH to the SDK.
DPI-aware terminal font scaling: Terminal text now scales correctly on high-DPI displays (125%, 150%, 200% scaling) and dynamically adjusts when moving windows between monitors with different DPI settings.
Related Issue
Fixes Windows users not being able to run installed system tools from Bash sessions and terminal text rendering issues on high-DPI displays.
Type of Change
Area
Commit Message Format
Follow conventional commits:
<type>: <subject>Types: feat, fix, docs, style, refactor, test, chore
Example:
feat: add user authentication systemChecklist
developbranchPlatform Testing Checklist
CRITICAL: This project supports Windows, macOS, and Linux. Platform-specific bugs are a common source of breakage.
platform/module instead of directprocess.platformchecksNote: Changes are Windows-specific and use the centralized platform module. CI will verify no regressions on macOS/Linux.
CI/Testing Requirements
Screenshots
N/A - No UI changes, backend/terminal rendering improvements only.
Feature Toggle
use_feature_nameBreaking Changes
Breaking: No
Details: Backward compatible improvements. No API changes.
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.