Skip to content

feat(projects): project settings editor + config-driven composer prefill (Phase 2)#3221

Open
serena-ruan wants to merge 3 commits into
mainfrom
projects-phase2-dialog
Open

feat(projects): project settings editor + config-driven composer prefill (Phase 2)#3221
serena-ruan wants to merge 3 commits into
mainfrom
projects-phase2-dialog

Conversation

@serena-ruan

@serena-ruan serena-ruan commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Related issue

Part of #863 (Projects epic — Phase 2, project defaults). Not closing the epic.

Summary

Wires up project-level session defaults end to end:

  • Project settings dialog (ProjectSettingsDialog, from the project-folder kebab) edits and persists a project's config — host, working directory, default agent, and an opt-in "Random worktree" toggle. Reuses the composer's host/agent pickers and filesystem browser. Worktrees are opt-in: the toggle defaults OFF and only an explicit ON stores use_worktree: true.
  • Composer prefill from stored config: a ?project= visit seeds host / working directory / agent from the project's stored config, silently falling back to the composer's generic defaults for any unset field. A stored sandbox default is honored (selects the sandbox, gated on managed sandboxes being enabled). An opt-in worktree generates a fresh worktree-<hex> branch once the workspace is in place.
  • Retired the newest-session inference (projectPrefill.ts collapsed to config-only; useNewestProjectSession removed) so stored config is the single source of truth — a project with no config prefills nothing project-specific.
  • Extracted the nested-dropdown dismiss guard into a dependency-free dialogDismissGuard.ts shared by the settings and scheduled-task dialogs.

Test Plan

  • Unit / component (vitest): projectPrefill.test.ts (config seeding, agent seeding, sandbox sentinel, load-order gates), ProjectSettingsDialog.test.tsx (seed/save round-trips, worktree opt-in, workspace disclosure), dialogDismissGuard.test.ts, updated NewChatDialog.projectPrefill.test.tsx. tsc clean, all suites green.
  • Backend (pytest): tests/stores/test_project_store.py — config persist / clear / unchanged.
  • E2E (playwright, tests/e2e_ui): test_project_settings_dialog.py (settings PATCH round-trip + clear), test_project_config_prefill.py (composer prefills host/workspace/agent from config; sandbox-default prefill — the regression the review flagged). All 4 pass against a real server.
  • Ran pre-commit run — passes.

Demo

Screen.Recording.2026-07-24.at.5.39.39.PM.mov

Type of change

  • Bug fix
  • Feature
  • UI / frontend change
  • Refactor / chore
  • Docs
  • Test / CI
  • Breaking change

Test coverage

  • Unit tests added / updated
  • Integration tests added / updated
  • E2E tests added / updated
  • Manual verification completed
  • Existing tests cover this change
  • Not applicable

Coverage notes

Manually verified in the running app: settings dialog persistence, composer prefill (host/workspace/agent/worktree), and the sandbox-default case. The working-directory / worktree prefill path can't be e2e-tested (the e2e_ui runner registers no real host workspace), so it's covered by component tests; everything else has automated unit + e2e coverage.

Changelog

Projects now carry default session settings (host, working directory, agent, optional git worktree) that pre-fill the new-session composer.

This pull request and its description were written by Isaac.

…ill (Phase 2)

Add a "Project settings" dialog to set a project's stored session defaults
(host, working directory, agent, opt-in random worktree) and wire the new-chat
composer to prefill from that stored config, retiring the newest-session
inference so stored config is the single source of truth.

- ProjectSettingsDialog: edit + persist config {host_id, workspace, agent_id,
  use_worktree}; worktrees opt-in (default OFF, store true when on). Reuses the
  composer's host/agent pickers and filesystem browser.
- projectPrefill: collapse to config-only seeding; unset fields fall through to
  the composer's generic defaults. Honor a stored sandbox default via
  selectSandbox (gated on managed sandboxes). Remove useNewestProjectSession.
- Extract the nested-dropdown dismiss guard into a dependency-free module shared
  by the settings and scheduled-task dialogs.

Co-authored-by: Isaac
Copilot AI review requested due to automatic review settings July 24, 2026 09:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added the size/XL Pull request size: XL label Jul 24, 2026
@serena-ruan

Copy link
Copy Markdown
Collaborator Author

/review

@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Benchmark results (SQLite, PR #3221)

Commit: 5f21e2f24f89930df50087318077a9b44a0f80f0

Benchmark comparison

Regression threshold: 100% on avg P50 or avg P95.

Journey Status Base P50 ms Cand P50 ms Δ P50 Base P95 ms Cand P95 ms Δ P95 Req/op
list_sessions ✅ ok 37.0 37.1 +0.3% 137.5 122.8 -10.7% 1.0
create_session ✅ ok 319.2 325.4 +1.9% 332.3 331.6 -0.2% 2.0
get_session ✅ ok 16.5 16.4 -1.0% 18.2 17.0 -6.4% 1.0
load_conversation_history ✅ ok 5.9 5.9 -0.6% 6.6 6.2 -5.8% 1.0
search_sessions ✅ ok 648.3 641.6 -1.0% 744.6 731.6 -1.7% 1.0
list_projects ✅ ok 31.3 30.5 -2.5% 130.1 115.8 -11.0% 1.0
list_project_sessions ✅ ok 54.4 51.3 -5.8% 173.1 137.5 -20.6% 1.0
fork_session ✅ ok 20.2 18.4 -8.6% 21.8 19.2 -11.7% 1.0
add_comment ✅ ok 5.3 5.1 -2.7% 6.4 5.4 -15.1% 1.0
session_cold_start ✅ ok 5987.2 6131.0 +2.4% 6041.2 6210.5 +2.8% 13.0
session_cold_restart ✅ ok 6231.4 6291.6 +1.0% 6322.3 6335.0 +0.2% 12.0
warm_turn ✅ ok 134.0 137.4 +2.5% 153.2 181.4 +18.4% 3.0
time_to_first_token ✅ ok 320.4 365.8 +14.2% 402.4 371.1 -7.8% 4.7→4.7
interrupt ✅ ok 117.9 124.1 +5.2% 132.9 167.8 +26.3% 5.0
read_runner_file ✅ ok 11.1 11.4 +2.1% 13.6 17.0 +25.3% 1.0

PASS — no regressions detected.

@omnigent-ci

omnigent-ci Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Blocking issues

1. Saving after a failed config load silently clears stored defaults (ProjectSettingsDialog.tsx). useProjectConfig is consumed here as only { data: stored, isLoading }. On a transient GET failure stored is undefined, so the open effect (const c = stored ?? {}) seeds an all-blank draft. Save is gated only on isLoading (disabled={updateConfig.isPending || isLoading}), so it's enabled, and onSubmit builds config from the empty draft → {}. Since the server treats {} as "clear," submitting after a failed load wipes an existing project's stored defaults. The dialog needs to distinguish "failed to load" from "no config" (surface the error / disable Save on isError for a first-class project) rather than treating a failed fetch as empty config.

2. One-shot prefill can settle permanently from stale React Query data (NewChatDialog.tsx + useConversations.ts). The prefill effect only proceeds once (prefillDone(prefill) short-circuits after settle), and it gates on isLoading / projectListLoading only — not staleness. useUpdateProjectConfig.onSuccess merely invalidates ["projects"] / ["project-config"] (it doesn't setQueryData), and both useProjects and useProjectConfig carry staleTime: 30_000. So immediately after saving settings, opening the composer for that project can resolve configProjectId from a stale ["projects"] snapshot (e.g. a label-only project still id: nullprefillConfig becomes {}) or read a stale old ["project-config"] value; the machine settles on that and the background refetch is then ignored because prefillDone is already true. Result: freshly-saved defaults are silently not applied on the next visit. Consider seeding the fresh config into the cache on mutation success, and/or letting the prefill re-run when the resolved config changes rather than latching on the first non-loading render.

Security vulnerabilities

No security issues found. The backend guard in sqlalchemy_store.py is sound: _encode_config rejects blobs over 64 KiB with INVALID_INPUT, and _decode_config coerces any non-dict JSON (from a manual DB edit or future writer) to {} so callers always get a mapping. config is persisted verbatim and never interpreted server-side (no injection/deserialization surface), and get/update remain owner-scoped through the existing store paths. Schemas (extra="forbid", config={} = clear vs None = unchanged) match the client contract. No lockfile or extras changes in this diff.

Non-blocking notes

  • Worktree opt-in silently no-ops on a probe failure (NewChatDialog.tsx). The new worktree effect dropped the hostWorktreesFailed / isError signal from useHostWorktrees. On a non-400 worktree-listing failure the effect just returns (hostWorktrees === undefined) and never generates a branch, so a project with use_worktree: true silently starts in the workspace with no worktree and no user-visible explanation. Acceptable (it correctly avoids blocking generic defaults), but worth a follow-up so the opt-in doesn't fail invisibly.
  • storedHostMissing fallback item correctly preserves an offline stored host so opening+saving doesn't drop it — good. Note that saving while that host is offline still re-persists it (intended), but the workspace field falls back to a plain path input in that state, which is the reasonable behavior.
  • Test coverage is strong (prefill config/agent/sandbox seeding, dialog round-trips, dismiss-guard helper, backend persist/clear), but neither blocking issue above is currently covered — add a stale-cache prefill test and a "load error → Save disabled / doesn't clear" test.

Summary

Solid, well-tested feature with a clean, dependency-free dismiss-guard extraction and a correct, security-conscious backend (size bound + non-dict coercion). Two blocking correctness/data-loss issues should be resolved before merge: (1) saving the settings dialog after a failed config GET clears existing defaults, and (2) the one-shot config-driven prefill can latch onto stale React Query data (especially right after a save, since the mutation only invalidates), so freshly-saved defaults may not apply on the next composer visit. Both are real given the mutation's invalidate-only cache handling and the 30s staleTime. The backend and the retirement of the newest-session inference are clean.

The review is complete and posted. That inbox item is just the cancellation confirmation for the claude_code reviewer I stopped. Let me drain it to clear the inbox.That was just the cancellation confirmation for the claude_code reviewer I stopped (it had drifted into PR-unrelated code after an unusually long run). The cross-vendor review is already complete and posted above, grounded in codex's independent findings plus my own read of the full diff.

The review flagged two blocking issues worth acting on before merge:

  1. Data loss — saving the project settings dialog after a failed config GET sends config: {}, clearing existing stored defaults.
  2. Stale-cache prefill race — the one-shot prefill can latch onto stale React Query data (the mutation only invalidates, with a 30s staleTime), so freshly-saved defaults may not apply on the next composer visit.

Backend (64 KiB bound + non-dict coercion) and the newest-session retirement are clean, with no security concerns.


Automated review by Polly · workflow run

…nference e2e

- Add useProjectConfig/useUpdateProjectConfig to all 10 Sidebar test mocks
  (Sidebar now mounts ProjectSettingsDialog, which calls them).
- Rewrite the settings-dialog e2e to create the project via POST /v1/projects
  instead of the flaky row-kebab move-to-project flow.
- Fix the composer-prefill e2e to stub GET /v1/sessions/projects (bare array),
  the real endpoint useProjects hits.
- Remove test_start_session_project_prefill — it exercised the newest-session
  inference path this PR retired; config-driven prefill replaces its coverage.

Co-authored-by: Isaac
Copilot AI review requested due to automatic review settings July 24, 2026 13:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

…resh prefill after save

Blocking issues from the PR review:

1. Data loss: saving the settings dialog after a failed config GET sent `{}`,
   which the server reads as "clear stored defaults". Now `useProjectConfig`'s
   isError is surfaced; a first-class project whose config failed to load blocks
   Save (with a notice), the seed effect skips a blank draft, and onSubmit bails.

2. Stale prefill after save: useUpdateProjectConfig only invalidated, so the
   composer's one-shot prefill could latch onto a stale cached config (30s
   staleTime) and drop just-saved defaults. It now setQueryData's the fresh
   config and upserts the projects list (so a promoted label-only folder
   resolves to its new id immediately).

Tests: dialog load-error blocks Save; hook seeds config + upserts list on
success; useProjectConfig disabled on null id and surfaces isError.

Co-authored-by: Isaac
Copilot AI review requested due to automatic review settings July 24, 2026 13:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Pull request size: XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants