Add Workspace Assistant Defaults V1#2316
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Review by Qodo
1.
|
There was a problem hiding this comment.
Code Review
This pull request implements Workspace Assistant Defaults V1, introducing backend storage, validation, and effective state resolution for Persona-backed defaults, alongside frontend store mappings, a settings UI modal in the Research Workspace header, and automatic inheritance of defaults in the Chat Workspace startup. The review feedback identifies a critical state synchronization issue where updating the default assistant in the settings modal only updates local component state instead of syncing with the global workspace store, which causes other workspace-scoped views to display stale defaults until a full reload.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/packages/ui/src/components/Option/ResearchWorkspace/__tests__/WorkspaceHeader.test.tsx`:
- Around line 187-217: The test helper createWorkspaceApiResponse mixes
snake_case and camelCase; update it to mirror the client shape returned by
tldwClient.getWorkspace() by renaming all snake_case fields to camelCase (e.g.,
studyMaterialsPolicy, bannerTitle, bannerSubtitle, bannerColor, audioProvider,
audioModel, audioVoice, audioSpeed, createdAt, lastModified) while keeping
existing camelCase keys (assistantDefaults, effectiveAssistantDefault) and the
...overrides spread so tests remain able to override values.
In
`@apps/packages/ui/src/components/Option/ResearchWorkspace/WorkspaceHeader.tsx`:
- Around line 604-626: The modal leaves stale state between opens causing
mismatches; in handleOpenDefaultAssistantModal before starting the fetch (after
the workspaceId check) clear/invalidate the modal-specific state by calling
setDefaultAssistantWorkspace(null), setDefaultAssistantPersonaId(null) and
setPersonaProfiles([]) (and any version field like
defaultAssistantWorkspace?.version if present) so the UI and save logic cannot
reuse previous workspace data; keep existing applyDefaultAssistantWorkspaceState
and setPersonaProfiles calls to repopulate on success and ensure the same reset
is applied in the error path/when closing the modal so failed or out-of-order
responses cannot apply the wrong persona/version.
- Around line 688-719: The success path after patching defaults must sync the
returned updatedWorkspace into the shared workspace store so runtime consumers
see the new assistantDefaults/effectiveAssistantDefault before the modal closes;
in handleClearDefaultAssistant (and the save path that calls
tldwClient.patchWorkspace) ensure you call the store updater (e.g.,
applyDefaultAssistantWorkspaceState(updatedWorkspace) or the appropriate
useWorkspaceStore setter) to write updatedWorkspace.assistantDefaults and
updatedWorkspace.effectiveAssistantDefault into the global store prior to
calling setDefaultAssistantModalOpen(false) and ending the flow.
In
`@apps/packages/ui/src/hooks/chat/__tests__/useChatActions.persona.integration.test.tsx`:
- Around line 345-360: The test is constructing a state shape that never occurs
in production because useMessageOption invokes useChatActions with
selectedAssistant set to effectiveSelectedAssistant; update the test to mirror
that payload by setting selectedAssistant to the inherited assistant (i.e. set
selectedAssistant to the same object as
inheritedAssistant/effectiveSelectedAssistant) rather than null, and keep
inheritedAssistant present so the hook sees the real-world shape; locate
references to useMessageOption, useChatActions, selectedAssistant and
inheritedAssistant in the test and make this substitution.
In `@apps/packages/ui/src/hooks/chat/personaServerChat.ts`:
- Around line 148-159: The personaMemoryMode computation incorrectly falls back
to newChatPersonaMemoryMode when serverChatId exists but serverChatMetaLoaded is
false, causing restored read_write mode to be overwritten; modify the logic in
the personaMemoryMode assignment (referencing newChatPersonaMemoryMode,
isMatchingPersonaChat, serverChatMetaLoaded, serverChatPersonaMemoryMode, and
resolvedServerChatId) so that if a resolvedServerChatId exists we preserve
serverChatPersonaMemoryMode (even while serverChatMetaLoaded is false) unless it
is explicitly undefined—i.e., treat the presence of a matching serverChatId as
sufficient to use serverChatPersonaMemoryMode and only fall back to
newChatPersonaMemoryMode when serverChatPersonaMemoryMode is null/undefined or
no matching server chat exists.
In `@apps/packages/ui/src/hooks/useMessageOption.tsx`:
- Around line 285-316: The current inheritedAssistant useMemo drops the
workspace-derived assistant as soon as serverChatId/history/messages appear,
losing provenance; change the logic to persist the first valid inherited
assistant instead of clearing it on subsequent state changes: create a ref
(e.g., inheritedAssistantRef) that captures opts.inheritedAssistant when it
first satisfies kind === "persona" and id != null and
effectiveAssistantState.mode === "plain" and no selectedAssistant, and then have
the inheritedAssistant memo return ref.current (if present) regardless of later
serverChatId/history/messages so long as selectedAssistant remains null; apply
the same ref-based preservation to the other similar memo referenced at lines
342-346 (the other inherited selection computation) so workspace provenance is
retained across renders.
In
`@apps/packages/ui/src/services/__tests__/tldw-api-client.workspace-api.test.ts`:
- Around line 513-565: Add a regression unit test for
workspaceApiMethods.patchWorkspace that verifies malformed assistantDefaults
input does not cause the client to send assistant_defaults: null or silently
succeed: call workspaceApiMethods.patchWorkspace with an invalid
assistantDefaults payload (e.g., wrong shape or null) and assert that either the
method throws or mocks.bgRequest was not called with a body containing
assistant_defaults: null; reference the existing test logic (mocks.bgRequest and
expected request object shape) and reuse the same pattern of
expect(mocks.bgRequest).toHaveBeenCalledWith/expect(...).toThrow to validate the
client prevents accidental clearing of assistant defaults.
In `@apps/packages/ui/src/services/tldw/domains/workspace-api.ts`:
- Around line 783-789: serializeWorkspaceAssistantDefaults currently converts
malformed non-null inputs into null via normalizeWorkspaceAssistantDefaults,
which leads callers to emit assistant_defaults: null and inadvertently clear
persisted defaults; instead, when normalizeWorkspaceAssistantDefaults(value)
returns a falsy/invalid result for a non-null input, fail fast by throwing a
descriptive error (or returning a distinct non-null sentinel that callers treat
as an error) rather than returning null. Update
serializeWorkspaceAssistantDefaults (and the similar serialization path around
the 875–890 region) to validate the normalized result and raise an exception
with context (including the original value and mention of assistant_defaults) so
invalid payloads cannot be coerced into a clear operation. Ensure callers that
assemble assistant_defaults do not treat thrown errors as nulls.
In `@tldw_Server_API/tests/Workspaces/test_workspaces_api.py`:
- Around line 69-89: Add a clear docstring to the helper function
_create_workspace_test_persona describing its purpose (create a test persona for
workspace tests), parameter types and meanings (db: CharactersRAGDB, persona_id,
user_id, name), return value (the created persona id/record returned by
db.create_persona_profile), and preconditions (expects
db.get_character_card_by_name("Test Char") to return a non-None character).
Include any side effects and note that it asserts the character exists before
calling db.create_persona_profile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84018666-52d2-44da-9637-3ee9a568510a
⛔ Files ignored due to path filters (1)
Docs/Product/Workspace_Persona_Defaults_PRD.mdis excluded by!docs/**
📒 Files selected for processing (32)
apps/packages/ui/src/components/Option/ChatWorkspace/ChatWorkspaceConsole.tsxapps/packages/ui/src/components/Option/ChatWorkspace/ChatWorkspacePage.tsxapps/packages/ui/src/components/Option/ChatWorkspace/InspectorRail.tsxapps/packages/ui/src/components/Option/ChatWorkspace/WorkspaceChatPanel.tsxapps/packages/ui/src/components/Option/ChatWorkspace/__tests__/ChatWorkspacePage.test.tsxapps/packages/ui/src/components/Option/ChatWorkspace/__tests__/InspectorRail.test.tsxapps/packages/ui/src/components/Option/ChatWorkspace/__tests__/WorkspaceChatPanel.test.tsxapps/packages/ui/src/components/Option/ChatWorkspace/types.tsapps/packages/ui/src/components/Option/ResearchWorkspace/WorkspaceHeader.tsxapps/packages/ui/src/components/Option/ResearchWorkspace/__tests__/WorkspaceHeader.test.tsxapps/packages/ui/src/hooks/chat/__tests__/personaServerChat.test.tsapps/packages/ui/src/hooks/chat/__tests__/useChatActions.persona.integration.test.tsxapps/packages/ui/src/hooks/chat/personaServerChat.tsapps/packages/ui/src/hooks/chat/useChatActions.tsapps/packages/ui/src/hooks/useMessageOption.tsxapps/packages/ui/src/services/__tests__/tldw-api-client.workspace-api.test.tsapps/packages/ui/src/services/tldw/domains/workspace-api.tsapps/packages/ui/src/store/__tests__/workspace.test.tsapps/packages/ui/src/store/workspace-slices/workspace-list-slice.tsapps/packages/ui/src/store/workspace.tsapps/packages/ui/src/types/workspace.tsbacklog/tasks/task-2278 - Implement-Workspace-Assistant-Defaults-backend-storage.mdbacklog/tasks/task-2318 - Plan-Workspace-Assistant-Defaults-implementation.mdbacklog/tasks/task-2318.1 - Map-Workspace-Assistant-Defaults-in-WebUI-client-and-store.mdbacklog/tasks/task-2318.2 - Expose-Workspace-Assistant-Defaults-API-validation-and-effective-state.mdbacklog/tasks/task-2318.3 - Add-Workspace-default-assistant-settings-UI.mdbacklog/tasks/task-2318.4 - Apply-Workspace-Persona-defaults-to-Chat-Workspace-startup.mdbacklog/tasks/task-2318.5 - Close-out-Workspace-Assistant-Defaults-V1-verification.mdtldw_Server_API/app/api/v1/endpoints/workspaces.pytldw_Server_API/app/api/v1/schemas/workspace_schemas.pytldw_Server_API/tests/Workspaces/test_workspace_assistant_defaults_api.pytldw_Server_API/tests/Workspaces/test_workspaces_api.py
05f8414 to
d7f2987
Compare
|
Addressed the open review feedback in d7f2987:
Verification after latest rebase onto dev:
Known unrelated verification blocker: full UI tsc with 8GB heap still fails in existing Notes test files for missing/changed props such as onCreateNote, listError, ServerCapabilities, and onSyncFolder; no Workspace Assistant Defaults files were implicated. |
There was a problem hiding this comment.
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/packages/ui/src/hooks/chat/personaServerChat.ts (1)
151-167: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUnhydrated chat reuse can attach a new persona turn to the wrong server chat.
Lines 151-163 treat any
resolvedServerChatIdwithserverChatMetaLoaded === falseas reusable. If a restored chat exists and the user switches personas before hydration finishes, this helper returns the stalechatId, rewrites local assistant state to the new persona, anduseChatActionssends the next turn into the old conversation. Reuse should require a verified persisted assistant match; otherwise reset/create a new chat first.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/packages/ui/src/hooks/chat/personaServerChat.ts` around lines 151 - 167, The unhydrated chat reuse path in personaServerChat’s persona memory/chat selection logic is too permissive and can route a new persona turn into the wrong persisted conversation. Update the resolved chat handling around isReusingUnhydratedChat, isMatchingPersonaChat, personaMemoryMode, and shouldResetServerChat so reuse only happens after a verified persisted assistant match; otherwise force a reset or create a new chat before useChatActions sends the next turn.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/packages/ui/src/components/Option/ChatWorkspace/WorkspaceChatPanel.tsx`:
- Around line 160-184: The assistant provenance logic in WorkspaceChatPanel is
reclassifying a restored inherited workspace persona as "explicit" when
useMessageOption no longer has a draft selection and only
serverChatAssistantKind/serverChatAssistantId remain. Update the assistantSource
determination to preserve "workspace" whenever the current chat is backed by the
inherited workspace persona, not just when selectedAssistantSource is
"workspace"; use the existing selectedMatchesWorkspaceAssistant,
usingWorkspaceAssistant, inheritedAssistant, and
serverChatAssistantKind/serverChatAssistantId checks together so reopened
sessions keep the workspace label and runtimeSelectedPersonaLabel stays
inherited.
In
`@apps/packages/ui/src/components/Option/ResearchWorkspace/WorkspaceHeader.tsx`:
- Around line 655-657: In WorkspaceHeader.tsx, the finally block in the default
assistant loading flow uses an early return for the stale-request guard, which
triggers noUnsafeFinally. Update the finally logic around
defaultAssistantRequestIdRef.current and setDefaultAssistantLoading so it uses a
conditional check instead of returning, while preserving the guard that only
clears loading for the active request.
In `@apps/packages/ui/src/hooks/useMessageOption.tsx`:
- Around line 255-271: The inherited assistant ref is being mutated during
render in useMessageOption, which can leave stale state behind if React abandons
a render. Move the write to inheritedAssistantRef.current out of the render path
and into an effect or other commit-safe place, keeping the
canCaptureInheritedAssistant logic in useMessageOption as the gate for when to
capture or clear the inherited assistant. Ensure assistantDraftSelection and
selectedAssistantSource only read from committed state, and preserve the
existing reset behavior when selectedAssistant is present.
In `@tldw_Server_API/app/api/v1/endpoints/workspaces.py`:
- Around line 169-176: The new db.get_persona_profile() calls in the workspace
endpoints are bypassing the existing db-to-HTTP error mapping and can leak
generic 500s. Wrap the Persona lookup in the same map_db_error_to_http(...)
handling used elsewhere in list_workspaces, get_workspace, patch_workspace, and
get_workspace_context, so ConflictError, InputError, and CharactersRAGDBError
are translated consistently before caching or returning the profile.
---
Outside diff comments:
In `@apps/packages/ui/src/hooks/chat/personaServerChat.ts`:
- Around line 151-167: The unhydrated chat reuse path in personaServerChat’s
persona memory/chat selection logic is too permissive and can route a new
persona turn into the wrong persisted conversation. Update the resolved chat
handling around isReusingUnhydratedChat, isMatchingPersonaChat,
personaMemoryMode, and shouldResetServerChat so reuse only happens after a
verified persisted assistant match; otherwise force a reset or create a new chat
before useChatActions sends the next turn.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f2fe600-cf1f-4711-a4dd-7b9d6d87c133
⛔ Files ignored due to path filters (1)
Docs/Product/Workspace_Persona_Defaults_PRD.mdis excluded by!docs/**
📒 Files selected for processing (34)
apps/packages/ui/src/components/Option/ChatWorkspace/ChatWorkspaceConsole.tsxapps/packages/ui/src/components/Option/ChatWorkspace/ChatWorkspacePage.tsxapps/packages/ui/src/components/Option/ChatWorkspace/InspectorRail.tsxapps/packages/ui/src/components/Option/ChatWorkspace/WorkspaceChatPanel.tsxapps/packages/ui/src/components/Option/ChatWorkspace/__tests__/ChatWorkspacePage.test.tsxapps/packages/ui/src/components/Option/ChatWorkspace/__tests__/InspectorRail.test.tsxapps/packages/ui/src/components/Option/ChatWorkspace/__tests__/WorkspaceChatPanel.test.tsxapps/packages/ui/src/components/Option/ChatWorkspace/types.tsapps/packages/ui/src/components/Option/ResearchWorkspace/WorkspaceHeader.tsxapps/packages/ui/src/components/Option/ResearchWorkspace/__tests__/WorkspaceHeader.test.tsxapps/packages/ui/src/hooks/__tests__/useMessageOption.assistant-overlay.test.tsxapps/packages/ui/src/hooks/chat/__tests__/personaServerChat.test.tsapps/packages/ui/src/hooks/chat/__tests__/useChatActions.persona.integration.test.tsxapps/packages/ui/src/hooks/chat/personaServerChat.tsapps/packages/ui/src/hooks/chat/useChatActions.tsapps/packages/ui/src/hooks/useMessageOption.tsxapps/packages/ui/src/services/__tests__/tldw-api-client.workspace-api.test.tsapps/packages/ui/src/services/tldw/domains/workspace-api.tsapps/packages/ui/src/store/__tests__/workspace.test.tsapps/packages/ui/src/store/workspace-slices/workspace-list-slice.tsapps/packages/ui/src/store/workspace.tsapps/packages/ui/src/types/workspace.tsbacklog/tasks/task-2278 - Implement-Workspace-Assistant-Defaults-backend-storage.mdbacklog/tasks/task-2318 - Plan-Workspace-Assistant-Defaults-implementation.mdbacklog/tasks/task-2318.1 - Map-Workspace-Assistant-Defaults-in-WebUI-client-and-store.mdbacklog/tasks/task-2318.2 - Expose-Workspace-Assistant-Defaults-API-validation-and-effective-state.mdbacklog/tasks/task-2318.3 - Add-Workspace-default-assistant-settings-UI.mdbacklog/tasks/task-2318.4 - Apply-Workspace-Persona-defaults-to-Chat-Workspace-startup.mdbacklog/tasks/task-2318.5 - Close-out-Workspace-Assistant-Defaults-V1-verification.mdbacklog/tasks/task-2318.6 - Address-Workspace-Assistant-Defaults-PR-review-feedback.mdtldw_Server_API/app/api/v1/endpoints/workspaces.pytldw_Server_API/app/api/v1/schemas/workspace_schemas.pytldw_Server_API/tests/Workspaces/test_workspace_assistant_defaults_api.pytldw_Server_API/tests/Workspaces/test_workspaces_api.py
d7f2987 to
4ab486d
Compare
|
Rebased PR #2316 onto current origin/dev (9445a17) and force-pushed head 4ab486d. GitHub now reports the PR as mergeable; fresh CI is pending. Local verification after rebase:
Typecheck note: plain |
There was a problem hiding this comment.
3 issues found across 35 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
671e808 to
0ee4797
Compare
PR Summary by Qodo
Add Workspace Assistant Defaults V1
✨ Enhancement🧪 Tests🕐 40+ MinutesWalkthroughs
User Description
Summary
Change summary
Maintainer note: this PR is materially AI-authored. Please replace or supplement this section with the required human-owned change summary before merge.
Verification
python -m pytest tldw_Server_API/tests/Workspaces/test_workspace_assistant_defaults_api.py tldw_Server_API/tests/Chat/unit/test_chat_conversations_api.py -q-> 22 passed.python -m pytest tldw_Server_API/tests/ChaChaNotesDB/test_workspace_assistant_defaults_db.py tldw_Server_API/tests/Workspaces/test_workspace_assistant_defaults_api.py tldw_Server_API/tests/Workspaces/test_workspaces_api.py tldw_Server_API/tests/ChaChaNotesDB/test_workspace_sub_resources_db.py -q-> 115 passed.vitest runfocused Workspace Assistant Defaults frontend suite -> 120 passed.tsc --noEmitinapps/packages/ui-> passed.git diff --check-> passed.banditon touched backend workspace schema/endpoint/DB scope -> no findings.AI Description
Diagram
graph TD subgraph Backend["Backend (Python)"] WS_EP["workspaces.py\nEndpoint"] --> WS_SCH["workspace_schemas.py\nSchemas"] WS_EP --> DB[("CharactersRAGDB")] end subgraph APIClient["API Client (TS)"] WS_API["workspace-api.ts\nNormalize / Serialize"] end subgraph Store["Workspace Store"] WS_STORE["workspace.ts\nState + Snapshots"] WS_LIST["workspace-list-slice.ts"] end subgraph ChatWorkspace["Chat Workspace UI"] CWP["ChatWorkspacePage"] --> CWC["ChatWorkspaceConsole"] CWC --> WCP["WorkspaceChatPanel"] CWC --> IR["InspectorRail"] WCP --> UMO["useMessageOption"] UMO --> UCA["useChatActions"] UCA --> PSC["personaServerChat"] end subgraph ResearchWorkspace["Research Workspace UI"] WH["WorkspaceHeader\nDefault Assistant Modal"] end DB -- "effective_assistant_default" --> WS_EP WS_EP -- "WorkspaceResponse" --> WS_API WS_API -- "normalized response" --> WS_STORE WS_STORE -- "effectiveAssistantDefault" --> CWP WS_STORE -- "assistantDefaults" --> WH WH -- "PATCH assistantDefaults" --> WS_API subgraph Legend direction LR _db[("Database")] ~~~ _svc["Service/Hook"] ~~~ _ui(["UI Component"]) endHigh-Level Assessment
The following are alternative approaches to this PR:
1. React context for effectiveAssistantDefault
Recommendation: The PR's approach — threading
effectiveAssistantDefaultfrom the store through props intoWorkspaceChatPanelanduseMessageOption— is sound and keeps the inheritance logic close to the chat submission path. One alternative worth considering is a dedicated React context for workspace assistant defaults rather than prop-drilling through Console → Panel, which would reduce the prop surface area. However, the current approach is consistent with how other workspace state (e.g.,backendAvailable,stagedSources) is already threaded, so deviating would be inconsistent. The decision to excludeeffectiveAssistantDefaultfrom persisted snapshots (only persistingassistantDefaults) is correct and avoids stale resolved labels in localStorage.File Changes
Enhancement (14)
Bug fix (1)
Tests (10)
Documentation (7)
Other (1)