feat(workbench-shell): ✨ integrate task boards and thread profiles - #98
Conversation
Adjust the runtime thread surface so the active task board is rendered outside the conversation stream and the chat content gains dynamic bottom padding to keep the final messages visible. This also keeps task history visible when present and updates the spacing logic so runtime errors continue to align correctly with the latest relevant timeline entry.
Add a unified stacked layout for the active task board and prompt composer so they visually connect as one surface. - Wrap the active task board in a matching shell with rounded top corners, blur, and border treatment - Allow the prompt composer to accept outer wrapper and shell class overrides for tighter layout control - Remove the visual gap between the task board and composer when a board is active while preserving the existing standalone composer appearance
Add thread-level profile storage and management so threads can keep their selected profile independently of runs. - Add a migration to introduce `threads.profile_id`, backfill existing threads from prior run/profile settings, and index the new column - Persist `profile_id` when creating threads and expose a command to update it later - Return `profile_id` in thread records and summaries throughout the backend - Update repository queries and tests to account for the new thread profile field - Adjust frontend types and runtime thread surface handling for the new profile data
AI Code Review SummaryPR: #98 (feat(workbench-shell): ✨ integrate task boards and thread profiles) Overall AssessmentDetected 12 actionable findings, prioritize CRITICAL/HIGH before merge. Major Findings by Severity
Actionable Suggestions
Potential Risks
Test Suggestions
File-Level Coverage Notes
Inline Downgraded Items (processed but not inline)
Coverage Status
Uncovered list:
No-patch covered list:
Runtime/Budget
|
| ) -> Result<Vec<ThreadRecord>, AppError> { | ||
| let rows = sqlx::query_as::<_, ThreadRow>( | ||
| "SELECT id, workspace_id, title, status, summary, last_active_at, created_at, updated_at | ||
| "SELECT id, workspace_id, profile_id, title, status, summary, last_active_at, created_at, updated_at |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| ...current, | ||
| [activeThread.id]: profileId, | ||
| })); | ||
| await threadUpdateProfile(activeThread.id, profileId); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| : (terminalThreadBindings[ | ||
| getNewThreadTerminalBindingKey(nextWorkspaceId) | ||
| ] ?? null); | ||
| let persistedThreadProfileId = activeAgentProfileId; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| portable-pty = "0.9" | ||
| git2 = { version = "0.20", features = ["vendored-libgit2", "vendored-openssl"] } | ||
| tiycore = "0.1.10" | ||
| tiycore = "0.1.11-rc.26042020" |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| let manager = ThreadManager::new(pool.clone()); | ||
|
|
||
| manager | ||
| .update_profile("t-update-profile", Some("profile-new")) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| expect( | ||
| resolveThreadProfileId("t-1", bindings, new Set(), globalActive, null), | ||
| ).toBe(globalActive); | ||
| it("preserves deleted profile ids instead of silently falling back", () => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| // default profile because it defines the profile new conversations inherit. | ||
| const handleSelectAgentProfileForThread = useCallback( | ||
| (profileId: string) => { | ||
| async (profileId: string) => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| ) | ||
| .bind(thread_id) | ||
| .bind(workspace_id) | ||
| .bind(profile_id) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Change nullish coalescing (`??`) to logical OR (`||`) in resolveThreadProfileId and resolveActiveThreadWorkbenchProfileId so empty-string profile IDs correctly fall back to the global active profile instead of being preserved as valid values - Add try/catch around threadUpdateProfile with user-facing error message via new i18n key dashboard.error.switchProfile - Add backend test verifying profile can be cleared to None - Add frontend tests for empty-string fallback behavior
| ) -> Result<Vec<ThreadRecord>, AppError> { | ||
| let rows = sqlx::query_as::<_, ThreadRow>( | ||
| "SELECT id, workspace_id, title, status, summary, last_active_at, created_at, updated_at | ||
| "SELECT id, workspace_id, profile_id, title, status, summary, last_active_at, created_at, updated_at |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| state.thread_manager.update_title(&id, &title).await | ||
| } | ||
|
|
||
| #[tauri::command] |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn update_profile( |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -1470,24 +1434,6 @@ export function DashboardWorkbench() { | |||
| await waitForBackendReady(); | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| portable-pty = "0.9" | ||
| git2 = { version = "0.20", features = ["vendored-libgit2", "vendored-openssl"] } | ||
| tiycore = "0.1.10" | ||
| tiycore = "0.1.11-rc.26042020" |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| assert_eq!(row.get::<String, _>("status"), "idle"); | ||
| } | ||
|
|
||
| #[tokio::test] |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| ) | ||
| .bind(thread_id) | ||
| .bind(workspace_id) | ||
| .bind(profile_id) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| expect( | ||
| resolveThreadProfileId(null, { "t-1": "p-2" }, profileIds, globalActive, firstProfile), | ||
| ).toBe(globalActive); | ||
| it("returns the global active profile when the thread has no persisted profile", () => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
|
|
||
| #[tauri::command] | ||
| pub async fn thread_update_profile( |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| // Intentional: new-thread mode always uses the global active profile as the | ||
| // default for the new conversation; switching to an existing thread reads | ||
| // its own persisted profile_id via resolveThreadProfileId instead. | ||
| let persistedThreadProfileId = activeAgentProfileId; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| sqlx::query( | ||
| "INSERT INTO threads (id, workspace_id, title, status, summary, last_active_at, created_at, updated_at) | ||
| VALUES (?, ?, ?, ?, ?, ?, ?, ?)", | ||
| "INSERT INTO threads (id, workspace_id, profile_id, title, status, summary, last_active_at, created_at, updated_at) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
|
|
||
| #[tauri::command] | ||
| pub async fn thread_update_profile( |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| [activeThread.id]: profileId, | ||
| })); | ||
| try { | ||
| await threadUpdateProfile(activeThread.id, profileId); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| if (matchedProfile) { | ||
| return matchedProfile; | ||
| } | ||
| return isNewThreadMode ? (agentProfiles[0] ?? null) : null; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
| return agentProfiles[0] ?? null; | ||
| }, [activeAgentProfileId, agentProfiles, allowMissingActiveProfile]); | ||
| const hasMissingActiveProfile = |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| state: State<'_, AppState>, | ||
| workspace_id: String, | ||
| title: Option<String>, | ||
| profile_id: Option<String>, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| /// Update the profile bound to a thread. | ||
| pub async fn update_profile(&self, id: &str, profile_id: Option<&str>) -> Result<(), AppError> { | ||
| let updated = thread_repo::update_profile(&self.pool, id, profile_id).await?; | ||
| if !updated { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| ).toBe("p-2"); | ||
| }); | ||
|
|
||
| it("falls back to firstProfileId when bound profile was deleted", () => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -1470,24 +1434,6 @@ export function DashboardWorkbench() { | |||
| await waitForBackendReady(); | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| ) -> Result<Vec<ThreadRecord>, AppError> { | ||
| let rows = sqlx::query_as::<_, ThreadRow>( | ||
| "SELECT id, workspace_id, title, status, summary, last_active_at, created_at, updated_at | ||
| "SELECT id, workspace_id, profile_id, title, status, summary, last_active_at, created_at, updated_at |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
|
|
||
| #[tauri::command] | ||
| pub async fn thread_update_profile( |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn update_profile( |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| if (matchedProfile) { | ||
| return matchedProfile; | ||
| } | ||
| return isNewThreadMode ? (agentProfiles[0] ?? null) : null; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| [activeThread.id]: profileId, | ||
| })); | ||
| try { | ||
| await threadUpdateProfile(activeThread.id, profileId); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| .await; | ||
| let manager = ThreadManager::new(pool.clone()); | ||
|
|
||
| manager |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| if (profileIds.has(boundId)) return boundId; | ||
| // Bound profile was deleted → fall back to first available profile. | ||
| return firstProfileId ?? globalActiveProfileId; | ||
| return threadProfileId || globalActiveProfileId; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| ) | ||
| .bind(thread_id) | ||
| .bind(workspace_id) | ||
| .bind(profile_id) |
There was a problem hiding this comment.
[LOW] Misaligned indentation on .bind(profile_id) in seed_thread
The newly added .bind(profile_id) line is indented 2 spaces less than the other .bind() calls in the same chain, creating inconsistent formatting.
Suggestion: Run cargo fmt --manifest-path src-tauri/Cargo.toml to auto-fix the indentation before committing, as required by the project post-implementation checklist.
Risk: Cosmetic only; cargo fmt will fix it, but if skipped the diff will contain noise.
Confidence: 0.95
| hasMissingActiveProfile | ||
| ? t("composer.profileDeletedHint") | ||
| : "Select an agent profile with an enabled model before starting a run.", | ||
| ); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| ? "assistant" | ||
| : lastPresentationRole; | ||
|
|
||
| const conversationBottomPadding = BASE_CONVERSATION_BOTTOM_PADDING; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ands Restructure command creation to always create entries locally first, then persist to backend only when a valid name is provided. This improves UX by allowing immediate local creation without waiting for async backend calls. - Add `pendingCreate` flag to `CommandEntry` type for tracking un-persisted entries - Add `inflightCreateIds` set to prevent duplicate backend calls - Simplify `removeCommand` to always remove locally first, skipping backend delete for pending entries - Simplify `addCommand` to create local-only entry immediately - Defer backend creation in `updateCommand` until name is non-empty - Change Rust prompt parsing to use `trim_end()` instead of `trim()` to preserve intentional leading whitespace
- Add defaultCollapsed prop to control initial collapse state - Replace static title div with interactive button containing chevron toggle - Conditionally render task list and status badge based on collapsed state - Animate chevron rotation on collapse/expand transition
| sqlx::query( | ||
| "INSERT INTO threads (id, workspace_id, title, status, summary, last_active_at, created_at, updated_at) | ||
| VALUES (?, ?, ?, ?, ?, ?, ?, ?)", | ||
| "INSERT INTO threads (id, workspace_id, profile_id, title, status, summary, last_active_at, created_at, updated_at) |
There was a problem hiding this comment.
[CRITICAL] Missing database migration for profile_id column on threads table
All thread_repo queries now reference a profile_id column that does not yet exist in the threads table. Without a corresponding migration in src-tauri/migrations/, any existing database will throw a SQL error at runtime.
Suggestion: Add a migration file (e.g., src-tauri/migrations/XXXX_add_thread_profile_id.sql) with: ALTER TABLE threads ADD COLUMN profile_id TEXT;
Risk: Runtime crash on any thread operation for existing installations; new installs may work if the schema is built from DDL that already includes the column, but existing users will break.
Confidence: 0.95
|
|
||
| seed_workspace(&pool, "ws-dup-approval", workspace_path.to_str().unwrap()).await; | ||
| seed_thread(&pool, "t-dup-approval", "ws-dup-approval").await; | ||
| seed_thread(&pool, "t-dup-approval", "ws-dup-approval", None).await; |
There was a problem hiding this comment.
[HIGH] All seed_thread call sites pass None for profile_id, leaving the new column path untested
While the schema and insert now support profile_id, no test actually inserts a thread with a non-null profile_id, meaning the SQL binding and retrieval for non-null profile_id is untested.
Suggestion: Add at least one integration test that seeds a thread with Some(profile_id) and verifies it is correctly persisted and returned via find_by_id or list_by_workspace.
Risk: A bug in profile_id binding or retrieval (e.g., null handling, type mismatch) would go undetected until runtime.
Confidence: 0.93
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn update_profile( |
There was a problem hiding this comment.
[HIGH] No test coverage for thread_update_profile command and repo function
The new update_profile repo function and thread_update_profile command have zero test coverage. The existing test file (agent.rs) only exercises seed_thread with None for profile_id.
Suggestion: Add integration tests for thread_update_profile covering: successful update (Some(profile_id)), clearing profile (None), and not-found error case. Also add a test for thread_create with a non-None profile_id.
Risk: The update_profile SQL could have binding issues, the not-found error path could be wrong, or the profile_id could be persisted incorrectly — none of these are verified.
Confidence: 0.95
| } | ||
|
|
||
| /** Track IDs of pending entries whose backend create call is in-flight. */ | ||
| const inflightCreateIds = new Set<string>(); |
There was a problem hiding this comment.
[HIGH] Module-level mutable inflightCreateIds singleton causes test pollution and flaky tests
A module-level mutable Set persists across test runs. If any test triggers command creation without awaiting the promise, stale IDs remain in the set, causing subsequent tests to skip backend calls they should exercise.
Suggestion: Either move inflightCreateIds inside the hook (useRef), expose a reset function for tests, or add an afterEach cleanup that clears the set. Add unit tests that verify the dedup logic works correctly under concurrent calls.
Risk: Silent test pollution where later tests skip backend creation because an ID leaked from a prior test, leading to false passes.
Confidence: 0.92
| agentProfiles[0] ?? | ||
| null, | ||
| [workbenchActiveProfileId, agentProfiles], | ||
| () => { |
There was a problem hiding this comment.
[HIGH] Missing unit tests for workbenchActiveAgentProfile returning null on deleted profiles
The useMemo now returns null for existing threads whose profile was deleted, rather than falling back. This is a key behavioral change with no dedicated unit test covering the null case, the new-thread fallback, or downstream consumers that receive null.
Suggestion: Extract the profile resolution logic into a testable pure function (similar to resolveThreadProfileId) and add unit tests for: existing thread with deleted profile → null, new thread mode with deleted profile → first profile, new thread mode with no profiles → null.
Risk: Downstream consumers may not handle null activeProfile correctly, causing runtime errors that only manifest in production when a user deletes a profile bound to an existing thread.
Confidence: 0.90
| argument_hint: meta.argument_hint.unwrap_or_default(), | ||
| description: meta.description.unwrap_or_default(), | ||
| prompt: body.trim().to_string(), | ||
| prompt: body.trim_end().to_string(), |
There was a problem hiding this comment.
[MEDIUM] Prompt trimming behavioral change lacks regression tests
Changing trim() to trim_end() preserves leading whitespace but removes trailing whitespace differently. Removing trim on input.prompt means user-supplied leading/trailing whitespace is now preserved. Neither change has test coverage.
Suggestion: Add unit tests verifying: (1) file-loaded prompts with leading whitespace are preserved after trim_end(), (2) input.prompt with leading/trailing whitespace is stored as-is, (3) the previous behavior (trim both ends) is intentionally no longer expected.
Risk: Subtle prompt formatting differences could affect agent behavior without being caught by tests.
Confidence: 0.88
| @@ -1295,67 +1302,41 @@ export function useSettingsController() { | |||
| }; | |||
|
|
|||
| const addCommand = (entry: Omit<CommandEntry, "id">) => { | |||
There was a problem hiding this comment.
[MEDIUM] Missing test for pendingCreate command lifecycle and deferred backend creation
The deferred command creation flow is complex: addCommand creates a local entry with pendingCreate flag, updateCommand may strip or keep it, and the backend call is triggered only when name becomes non-empty and not already in-flight. None of these paths have unit tests.
Suggestion: Add unit tests for: addCommand in Tauri mode sets pendingCreate=true; updateCommand with empty name keeps pendingCreate; updateCommand with non-empty name triggers backend create and removes pendingCreate; inflightCreateIds dedup prevents duplicate calls; failed creation cleans up inflightCreateIds; removeCommand on pending entry skips backend delete.
Risk: Edge cases like double-updating a pending command before the first create resolves, or removing a pending command while its create is in-flight, could lead to orphaned backend entries or UI inconsistency.
Confidence: 0.87
| // default profile because it defines the profile new conversations inherit. | ||
| const handleSelectAgentProfileForThread = useCallback( | ||
| (profileId: string) => { | ||
| async (profileId: string) => { |
There was a problem hiding this comment.
[MEDIUM] Missing test for async handleSelectAgentProfileForThread with threadUpdateProfile error path
The profile switch handler is now async with a backend call and error handling. Neither the success path (workspace state update + override set) nor the failure path (composerError display) has test coverage.
Suggestion: Add integration or unit tests covering: successful profile switch updates workspace thread profileId and override; failed threadUpdateProfile sets composerError and does not mutate state; the function is not called when activeThread?.id is null.
Risk: A failed threadUpdateProfile call silently swallows the error if the catch path has a bug, or leaves the UI in an inconsistent state where local override is set but backend disagrees.
Confidence: 0.88
| } | ||
|
|
||
| const creationPromise = threadCreate(workspaceId, "") | ||
| const creationPromise = threadCreate(workspaceId, "", activeAgentProfileId) |
There was a problem hiding this comment.
[MEDIUM] Missing test for thread creation profile assignment and workspace state update
threadCreate now receives activeAgentProfileId as a third argument. After creation, the code updates both the profile override and the workspace thread's profileId. Neither the argument passing nor the state update has test coverage, and the fallback thread.profileId ?? activeAgentProfileId handles a null response that isn't tested.
Suggestion: Add tests verifying: activeAgentProfileId is passed to threadCreate; workspace threads are updated with the returned profileId; fallback to activeAgentProfileId when thread.profileId is null.
Risk: If the backend doesn't return profileId, the fallback logic may create a mismatch between the local override and the workspace thread object.
Confidence: 0.85
| * persisted thread-level profile_id; deleted profile ids are preserved so the | ||
| * UI can render a missing-profile state instead of silently falling back. | ||
| */ | ||
| export function resolveThreadProfileId( |
There was a problem hiding this comment.
[LOW] resolveThreadProfileId and resolveActiveThreadWorkbenchProfileId are now identical
After the refactor, both resolve functions have identical logic (threadProfileId || globalActiveProfileId). They should be consolidated to reduce duplication and avoid divergence.
Suggestion: Replace one function with the other or create a single shared helper. Remove the unused export and update callers.
Risk: The two functions could diverge in future edits, causing subtle bugs.
Confidence: 0.95
Summary
tiycoreto0.1.11-rc.26042020.Test Plan
npm run typechecknpm run test:unit -- dashboard-workbench.test.tscargo fmt --manifest-path src-tauri/Cargo.tomlcargo test --manifest-path src-tauri/Cargo.toml --test m1_4_thread -- --nocapture🤖 Generated with TiyCode