feat(settings): add environments settings#1361
Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds a Settings → Environments feature: a derived SQLite Changes
Sequence Diagram(s)sequenceDiagram
participant User as Renderer (User)
participant Store as Project Store
participant Presenter as Project Presenter
participant DB as SQLite (new_environments)
rect rgba(200,200,255,0.5)
User->>Store: fetchEnvironments()
Store->>Presenter: getEnvironments()
Presenter->>DB: newEnvironmentsTable.list()
DB-->>Presenter: [EnvironmentRow...]
Presenter->>Presenter: map rows -> EnvironmentSummary (fs.exists)
Presenter-->>Store: [EnvironmentSummary...]
Store-->>User: render environments
end
sequenceDiagram
participant SessionMgr as Session Manager
participant EnvTable as NewEnvironmentsTable
participant DB as SQLite
rect rgba(200,255,200,0.5)
SessionMgr->>EnvTable: syncPath(projectDir) on create/update/delete
EnvTable->>DB: aggregate session counts & last_used_at
DB-->>EnvTable: results
EnvTable->>DB: upsert/delete rows in new_environments
end
sequenceDiagram
participant MainConfig as ConfigPresenter (Main)
participant IPC as IPC/Event Bus
participant Renderer as Renderer Process
rect rgba(255,230,200,0.5)
MainConfig->>MainConfig: setDefaultProjectPath(path)
MainConfig->>IPC: emit DEFAULT_PROJECT_PATH_CHANGED
IPC->>Renderer: CONFIG_EVENTS.DEFAULT_PROJECT_PATH_CHANGED
Renderer->>Renderer: projectStore.loadDefaultProjectPath() / reconcile
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 175dc292dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+192
to
+196
| ...projectStore.environments.filter( | ||
| (environment) => | ||
| (!environment.isTemp || environment.path === defaultProjectPath.value) && | ||
| (showMissing.value || environment.exists) | ||
| ), |
There was a problem hiding this comment.
This filter permanently excludes every isTemp entry unless it is already the default, and the component only defines showMissing—there is no companion state or control that can ever flip temp entries back on. As a result, temp workspaces never appear in this page, so users cannot review or reopen them even though this settings view is supposed to expose a collapsed temp section.
Useful? React with 👍 / 👎.
Comment on lines
+29
to
+33
| SELECT | ||
| acp.conversation_id AS session_id, | ||
| acp.workdir AS path, | ||
| MAX(COALESCE(ns.updated_at, 0), COALESCE(acp.updated_at, 0)) AS activity_at | ||
| FROM acp_sessions acp |
There was a problem hiding this comment.
For ACP-backed sessions without project_dir, last_used_at is derived from MAX(ns.updated_at, acp.updated_at). AcpSessionPersistence.saveSessionData() updates acp_sessions.updated_at whenever an ACP session is re-established, so merely reopening/rehydrating an ACP session will make that directory look "recent" even when no conversation activity happened. Because the settings page sorts by lastUsedAt, this causes environment history to drift away from actual chat usage.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
src/main/presenter/configPresenter/index.ts (1)
1877-1883: Skip redundant default-path broadcasts when value is unchanged.On Line 1879, the setter writes and emits every time. Guarding unchanged values avoids unnecessary renderer churn.
♻️ Suggested tweak
setDefaultProjectPath(projectPath: string | null): void { const normalized = projectPath?.trim() ? projectPath.trim() : null + const current = this.getDefaultProjectPath() + if (current === normalized) { + return + } this.setSetting('defaultProjectPath', normalized) eventBus.sendToRenderer(CONFIG_EVENTS.DEFAULT_PROJECT_PATH_CHANGED, SendTarget.ALL_WINDOWS, { path: normalized }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/configPresenter/index.ts` around lines 1877 - 1883, The setter setDefaultProjectPath currently always calls setSetting and broadcasts CONFIG_EVENTS.DEFAULT_PROJECT_PATH_CHANGED; change it to first normalize the incoming projectPath (as done now), read the existing value via getSetting('defaultProjectPath') (or the appropriate accessor), compare the existing value to the normalized value (treating null/empty equivalently), and if they are equal simply return; only call this.setSetting('defaultProjectPath', normalized) and eventBus.sendToRenderer(CONFIG_EVENTS.DEFAULT_PROJECT_PATH_CHANGED, SendTarget.ALL_WINDOWS, { path: normalized }) when the value actually changed.src/main/presenter/newAgentPresenter/sessionManager.ts (1)
67-87: Consider syncing paths only whenprojectDircan change.For updates that only change title/pin/draft, the extra path sync loop adds unnecessary DB work.
♻️ Suggested optimization
update( id: string, fields: Partial<Pick<SessionRecord, 'title' | 'projectDir' | 'isPinned' | 'isDraft'>> ): void { @@ this.sqlitePresenter.newSessionsTable.update(id, dbFields) + if (fields.projectDir === undefined) { + return + } + for (const path of this.sqlitePresenter.newEnvironmentsTable.listPathsForSession(id)) { affectedPaths.add(path) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/newAgentPresenter/sessionManager.ts` around lines 67 - 87, The code currently always gathers and syncs environment paths after calling newSessionsTable.update, causing unnecessary work when only title/isPinned/isDraft change; modify the logic to only compute affectedPaths and call newEnvironmentsTable.syncPath when fields.projectDir is provided and actually changes the session's project_dir value: fetch the current session row (e.g., via this.sqlitePresenter.newSessionsTable.get(id) or equivalent), compare its project_dir to fields.projectDir, and only if they differ run newEnvironmentsTable.listPathsForSession(id) to build affectedPaths and call newEnvironmentsTable.syncPath(path) for each path.src/main/presenter/projectPresenter/index.ts (1)
43-53: Consider async existence checks for large environment lists.
fs.existsSyncis synchronous and could block the event loop if the environments list grows large or if paths are on slow network drives. For typical usage this is likely fine, but worth noting.♻️ Optional: Use async fs.promises.access for non-blocking checks
+import { promises as fsp } from 'fs' + async getEnvironments(): Promise<EnvironmentSummary[]> { const rows = this.sqlitePresenter.newEnvironmentsTable.list() - return rows.map((row) => ({ - path: row.path, - name: path.basename(row.path) || row.path, - sessionCount: row.session_count, - lastUsedAt: row.last_used_at, - isTemp: this.isTempPath(row.path), - exists: fs.existsSync(row.path) - })) + return Promise.all( + rows.map(async (row) => ({ + path: row.path, + name: path.basename(row.path) || row.path, + sessionCount: row.session_count, + lastUsedAt: row.last_used_at, + isTemp: this.isTempPath(row.path), + exists: await fsp.access(row.path).then(() => true).catch(() => false) + })) + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/projectPresenter/index.ts` around lines 43 - 53, getEnvironments currently uses the synchronous fs.existsSync which can block the event loop for large lists; change it to perform async existence checks (e.g., use fs.promises.access or fs.promises.stat) and await them with Promise.all so the method remains non-blocking. Specifically, update the getEnvironments method in projectPresenter to map rows to async checks (retain fields: path, name via path.basename, session_count, last_used_at, isTemp via isTempPath) and return Promise<EnvironmentSummary[]> after awaiting all existence checks; ensure you import/use fs.promises and preserve the existing property names (sessionCount, lastUsedAt, isTemp, exists) in the returned objects.test/main/presenter/sqlitePresenter/newEnvironmentsTable.test.ts (1)
58-78: Minor: Consider null-safe access fordbin test bodies.While
beforeEachguaranteesdbis initialized, accessingdb.prepare(...)directly without a null check (line 59, 62, etc.) relies on test ordering. The TypeScript typeInstanceType<...> | nulltechnically allows null.This is a minor concern since the tests will always run after
beforeEach, but adding a non-null assertion or guard could improve type safety.💡 Option: Use non-null assertion in test body
it('syncPath aggregates only non-draft sessions for a single directory', () => { - db.prepare( + db!.prepare( 'INSERT INTO new_sessions (id, project_dir, is_draft, updated_at) VALUES (?, ?, ?, ?)' ).run('s1', '/work/app', 0, 100)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/sqlitePresenter/newEnvironmentsTable.test.ts` around lines 58 - 78, The test accesses the possibly-null typed variable db directly in the test body (calls to db.prepare(...)) even though beforeEach initializes it; to fix, make the access null-safe by using a non-null assertion (e.g., db!) or an explicit guard before using db, so update the calls in this test (and similar tests) that invoke db.prepare(...) to use db! or check db !== null; this keeps the existing logic (involving table.syncPath and table.list) but satisfies TypeScript's InstanceType<...> | null typing and avoids nullable access warnings.src/main/presenter/sqlitePresenter/tables/newEnvironments.ts (2)
184-186: Consider removingsyncForSessionwrapper.
syncForSessionsimply delegates tosyncSessionPathswithout any additional logic. Unless this is intended as a future extension point, it adds unnecessary indirection.💡 Option: Remove if not needed for API stability
If both methods need to exist for interface compliance or future extensibility, this is fine. Otherwise, consider consolidating to reduce maintenance surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/sqlitePresenter/tables/newEnvironments.ts` around lines 184 - 186, The method syncForSession is a redundant wrapper around syncSessionPaths (it only calls this.syncSessionPaths(sessionId)); remove syncForSession and update any callers to call syncSessionPaths(sessionId) directly, or if it must remain for interface compatibility, add a comment noting it’s intentionally passthrough; search for syncForSession usages and replace them with syncSessionPaths to consolidate and delete the now-unnecessary syncForSession function.
148-176: Minor: RedundantDISTINCTin outer query.The inner
UNION(without ALL) already removes duplicates between the two branches. The outerSELECT DISTINCTis redundant.♻️ Remove redundant DISTINCT
.prepare( ` - SELECT DISTINCT path + SELECT path FROM ( SELECT project_dir AS path🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/sqlitePresenter/tables/newEnvironments.ts` around lines 148 - 176, In listPathsForSession in newEnvironments.ts the outer SELECT DISTINCT is redundant because the inner UNION already deduplicates; remove the outer DISTINCT wrapper and change the query to directly select path from the subquery (e.g., "SELECT path FROM ( ... )") or even better collapse to a single UNION query without an extra outer SELECT, keeping the same parameter bindings for sessionId in the call to .all(sessionId, sessionId) and preserving the trimming/null checks in both branches.src/main/presenter/sqlitePresenter/index.ts (1)
504-511: Variablepathshadows importedpathmodule.The loop variable
path(lines 506, 509, 510) shadows the importedpathmodule from line 2. While this doesn't cause runtime issues in this context, it could lead to confusion during maintenance.♻️ Rename loop variable to avoid shadowing
const affectedPaths = new Set(this.newEnvironmentsTable.listPathsForSession(conversationId)) await this.acpSessionsTable.upsert(conversationId, agentId, data) - for (const path of this.newEnvironmentsTable.listPathsForSession(conversationId)) { - affectedPaths.add(path) + for (const envPath of this.newEnvironmentsTable.listPathsForSession(conversationId)) { + affectedPaths.add(envPath) } - for (const path of affectedPaths) { - this.newEnvironmentsTable.syncPath(path) + for (const envPath of affectedPaths) { + this.newEnvironmentsTable.syncPath(envPath) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/sqlitePresenter/index.ts` around lines 504 - 511, The loop variable named "path" shadows the imported "path" module; update the loops that iterate newEnvironmentsTable.listPathsForSession(conversationId) and the affectedPaths iteration to use a non-conflicting name (e.g., envPath or sessionPath) so the module import remains accessible and code is clearer; adjust all references inside those loops (uses of path) and keep the rest of the logic (calls to newEnvironmentsTable.syncPath and adding to affectedPaths / using affectedPaths) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/newAgentPresenter/legacyImportService.ts`:
- Line 538: The call to
this.sqlitePresenter.newEnvironmentsTable.rebuildFromSessions() can throw after
the import commit and should not cause the whole import to be reported as
failed; wrap the call in a try/catch inside the import completion path (around
where rebuildFromSessions() is invoked), log the error (including the error
object/message) via the existing logger/presenter (e.g., processLogger or
this.logger) and do not rethrow—ensure the import status remains success even if
rebuildFromSessions() fails. Also add a short comment referencing
newEnvironmentsTable.rebuildFromSessions to explain why failures are non-fatal.
In `@src/renderer/settings/components/EnvironmentsSettings.vue`:
- Around line 179-188: The synthetic default environment object in
EnvironmentsSettings.vue is setting exists: true without verifying the
filesystem; update the code that creates the synthetic default (the object with
properties path, name, isSyntheticDefault) to determine existence by asking the
main process or existing utility (e.g., invoke an IPC like
ipcRenderer.invoke('pathExists', path) or call the app's path-existence helper)
and set exists accordingly, or if you intentionally accept the limitation, add a
concise comment next to isSyntheticDefault explaining that exists is
optimistically true and why; modify the creation site so the exists field
reflects the actual filesystem check or is documented as intentional.
In `@src/renderer/src/i18n/ru-RU/settings.json`:
- Around line 1400-1402: Update the Russian UI text for the environments
description (JSON key "description" in the environments section of
settings.json) to fix awkward phrasing "назначайте один каталогом по умолчанию
для новых чатов"; replace it with a grammatically correct phrase such as
"назначайте один каталог в качестве каталога по умолчанию для новых чатов" so
the sentence reads naturally in the UI.
---
Nitpick comments:
In `@src/main/presenter/configPresenter/index.ts`:
- Around line 1877-1883: The setter setDefaultProjectPath currently always calls
setSetting and broadcasts CONFIG_EVENTS.DEFAULT_PROJECT_PATH_CHANGED; change it
to first normalize the incoming projectPath (as done now), read the existing
value via getSetting('defaultProjectPath') (or the appropriate accessor),
compare the existing value to the normalized value (treating null/empty
equivalently), and if they are equal simply return; only call
this.setSetting('defaultProjectPath', normalized) and
eventBus.sendToRenderer(CONFIG_EVENTS.DEFAULT_PROJECT_PATH_CHANGED,
SendTarget.ALL_WINDOWS, { path: normalized }) when the value actually changed.
In `@src/main/presenter/newAgentPresenter/sessionManager.ts`:
- Around line 67-87: The code currently always gathers and syncs environment
paths after calling newSessionsTable.update, causing unnecessary work when only
title/isPinned/isDraft change; modify the logic to only compute affectedPaths
and call newEnvironmentsTable.syncPath when fields.projectDir is provided and
actually changes the session's project_dir value: fetch the current session row
(e.g., via this.sqlitePresenter.newSessionsTable.get(id) or equivalent), compare
its project_dir to fields.projectDir, and only if they differ run
newEnvironmentsTable.listPathsForSession(id) to build affectedPaths and call
newEnvironmentsTable.syncPath(path) for each path.
In `@src/main/presenter/projectPresenter/index.ts`:
- Around line 43-53: getEnvironments currently uses the synchronous
fs.existsSync which can block the event loop for large lists; change it to
perform async existence checks (e.g., use fs.promises.access or
fs.promises.stat) and await them with Promise.all so the method remains
non-blocking. Specifically, update the getEnvironments method in
projectPresenter to map rows to async checks (retain fields: path, name via
path.basename, session_count, last_used_at, isTemp via isTempPath) and return
Promise<EnvironmentSummary[]> after awaiting all existence checks; ensure you
import/use fs.promises and preserve the existing property names (sessionCount,
lastUsedAt, isTemp, exists) in the returned objects.
In `@src/main/presenter/sqlitePresenter/index.ts`:
- Around line 504-511: The loop variable named "path" shadows the imported
"path" module; update the loops that iterate
newEnvironmentsTable.listPathsForSession(conversationId) and the affectedPaths
iteration to use a non-conflicting name (e.g., envPath or sessionPath) so the
module import remains accessible and code is clearer; adjust all references
inside those loops (uses of path) and keep the rest of the logic (calls to
newEnvironmentsTable.syncPath and adding to affectedPaths / using affectedPaths)
unchanged.
In `@src/main/presenter/sqlitePresenter/tables/newEnvironments.ts`:
- Around line 184-186: The method syncForSession is a redundant wrapper around
syncSessionPaths (it only calls this.syncSessionPaths(sessionId)); remove
syncForSession and update any callers to call syncSessionPaths(sessionId)
directly, or if it must remain for interface compatibility, add a comment noting
it’s intentionally passthrough; search for syncForSession usages and replace
them with syncSessionPaths to consolidate and delete the now-unnecessary
syncForSession function.
- Around line 148-176: In listPathsForSession in newEnvironments.ts the outer
SELECT DISTINCT is redundant because the inner UNION already deduplicates;
remove the outer DISTINCT wrapper and change the query to directly select path
from the subquery (e.g., "SELECT path FROM ( ... )") or even better collapse to
a single UNION query without an extra outer SELECT, keeping the same parameter
bindings for sessionId in the call to .all(sessionId, sessionId) and preserving
the trimming/null checks in both branches.
In `@test/main/presenter/sqlitePresenter/newEnvironmentsTable.test.ts`:
- Around line 58-78: The test accesses the possibly-null typed variable db
directly in the test body (calls to db.prepare(...)) even though beforeEach
initializes it; to fix, make the access null-safe by using a non-null assertion
(e.g., db!) or an explicit guard before using db, so update the calls in this
test (and similar tests) that invoke db.prepare(...) to use db! or check db !==
null; this keeps the existing logic (involving table.syncPath and table.list)
but satisfies TypeScript's InstanceType<...> | null typing and avoids nullable
access warnings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c603b058-05ec-41cb-936e-8041c4b7c3ec
📒 Files selected for processing (51)
docs/specs/settings-environments/plan.mddocs/specs/settings-environments/spec.mdsrc/main/events.tssrc/main/presenter/configPresenter/index.tssrc/main/presenter/index.tssrc/main/presenter/newAgentPresenter/legacyImportService.tssrc/main/presenter/newAgentPresenter/sessionManager.tssrc/main/presenter/projectPresenter/index.tssrc/main/presenter/sqlitePresenter/index.tssrc/main/presenter/sqlitePresenter/tables/newEnvironments.tssrc/renderer/settings/components/DashboardSettings.vuesrc/renderer/settings/components/EnvironmentsSettings.vuesrc/renderer/settings/main.tssrc/renderer/src/events.tssrc/renderer/src/i18n/da-DK/routes.jsonsrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/routes.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/routes.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/routes.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/routes.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/routes.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/routes.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/routes.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/routes.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/routes.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/routes.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/routes.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/stores/ui/project.tssrc/renderer/src/views/ChatTabView.vuesrc/shared/types/agent-interface.d.tssrc/shared/types/presenters/legacy.presenters.d.tssrc/shared/types/presenters/project.presenter.d.tstest/main/presenter/newAgentPresenter/legacyImportService.test.tstest/main/presenter/newAgentPresenter/sessionManager.test.tstest/main/presenter/projectPresenter/projectPresenter.test.tstest/main/presenter/sqlitePresenter.test.tstest/main/presenter/sqlitePresenter/newEnvironmentsTable.test.tstest/renderer/components/EnvironmentsSettings.test.tstest/renderer/components/NewThreadPage.test.tstest/renderer/stores/projectStore.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit