refactor(chat): remove manual view switching - #16739
Conversation
…ePage - Changed session layout preference from 'agent.layout' to 'agent.session.display_mode' in AgentPage. - Updated related tests to reflect the new session display mode. - Refactored Sessions component to remove unused session display options and history button. - Adjusted HomePage to use 'topic.tab.display_mode' instead of 'topic.layout' for topic display preferences. - Removed manual chat layout switches from CommonSettings. - Updated tests across various components to ensure consistency with the new display mode preferences.
…or for right panel
…o ResourceEntityRail test
… related tests to maintain sidebar behavior
…sourceList, and AssistantResourceList components
… enhance ConversationPickerDialog styling
…aderNavbar components
…hancing avatar display options
Signed-off-by: kangfenmao <kangfenmao@qq.com>
There was a problem hiding this comment.
This review was translated automatically.
Findings:
- Important:
src/renderer/pages/agents/AgentPage.tsx:467andsrc/renderer/pages/agents/AgentPage.tsx:894can both create a new blank agent session without a single-flight guard. The picker path closes the dialog before awaiting, but the composer path remains clickable, and either path can be triggered again before the firstPOST /agent-sessionsresult is reflected inclassicLayoutSessions. Because both code paths only callfindReusableEmptySessionagainst that stale list, rapid repeated actions can create duplicate empty sessions for the same agent/workspace. Home already guards the analogous classic topic paths withisCreatingTopicRef; please add the same protection for classic agent session creation and cover the rapid double-click/re-entry case.
Open questions:
- None.
Summary:
Requesting changes for the duplicate blank-session creation path in the new classic agent flow.
Verification:
- Reviewed PR #16739 at head
5e45378eeb71749758607bd8f7db2ce285427175in/Users/cherryai004/Documents/Cherry_Studio_Dev/cherry-studio-pr-review/.codex-tmp/pr-review-sweep-16739-20260704T090238Z. - Inspected the preference/display-mode migration, resource rail/menu changes, shell/pane/composer changes, edit dialog changes, docs, i18n, and related tests.
- Checked GitHub CI via
gh; latest relevant checks were successful or skipped by change gating. - Did not run local
pnpm lint/test/format, per repository PR review instructions.
After fixing, please request eeee0717 for further review.
Original Content
Findings:
- Important:
src/renderer/pages/agents/AgentPage.tsx:467andsrc/renderer/pages/agents/AgentPage.tsx:894can both create a new blank agent session without a single-flight guard. The picker path closes the dialog before awaiting, but the composer path remains clickable, and either path can be triggered again before the firstPOST /agent-sessionsresult is reflected inclassicLayoutSessions. Because both code paths only callfindReusableEmptySessionagainst that stale list, rapid repeated actions can create duplicate empty sessions for the same agent/workspace. Home already guards the analogous classic topic paths withisCreatingTopicRef; please add the same protection for classic agent session creation and cover the rapid double-click/re-entry case.
Open questions:
- None.
Summary:
Requesting changes for the duplicate blank-session creation path in the new classic agent flow.
Verification:
- Reviewed PR #16739 at head
5e45378eeb71749758607bd8f7db2ce285427175in/Users/cherryai004/Documents/Cherry_Studio_Dev/cherry-studio-pr-review/.codex-tmp/pr-review-sweep-16739-20260704T090238Z. - Inspected the preference/display-mode migration, resource rail/menu changes, shell/pane/composer changes, edit dialog changes, docs, i18n, and related tests.
- Checked GitHub CI via
gh; latest relevant checks were successful or skipped by change gating. - Did not run local
pnpm lint/test/format, per repository PR review instructions.
修复后请 request eeee0717 再进行后续 review。
AtomsH4
left a comment
There was a problem hiding this comment.
Reviewed the resource menu/layout preference refactor. The preference/source-of-truth migration is mostly consistent, but I left two warnings: one real deletion-guard race in the new classic rail clear-topics path, and one missing v2 breaking-changes entry for the user-visible layout behavior change.
0c90da6 to
5e45378
Compare
…-layout interactions
…en no assistant is selected
…ment after clearing assistant topics
Signed-off-by: kangfenmao <kangfenmao@qq.com>
Keyboard activation (Enter/Space at the listbox level) goes through the list's selectItem action, which the rail never wired to onSelectItem — so keyboard users could neither select an entity nor trigger the "activate the already-selected entity to toggle its pane" behavior. Wire onSelectItem to reuse handleItemClick, and drop the row's now-redundant onClick so mouse click, row-focused Enter/Space, and listbox-level keyboard all funnel through selectItem -> onSelectItem -> handleItemClick exactly once. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
The classic agent rail and agent session sidebar read/wrote assistant.icon_type, so changing the agent entity icon style also changed the assistant's (and vice versa). Add a v2-only agent.icon_type preference (default emoji, generated from target-key-definitions.json) and point AgentResourceList and Sessions at it. The icon-style option labels (emoji/model/none) stay shared: the text is surface-agnostic and already routed through the neutral RESOURCE_ICON_TYPE_* helpers, and the parent menu label is already agent-scoped (agent.icon.type). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
…stub The keyboard-navigation test stubs scrollToIndex on the mocked virtual list's ref node; the ref callback param is typed HTMLDivElement, so assign through a narrowed cast to keep tsgo happy (the pre-commit hook does not run typecheck). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
…tion After deleting the active topic/session the two surfaces disagreed: topic and classic-session deletes picked the neighbour in the scoped visible list, but the modern session sidebar fell back to sessionItems.find(s => s.id !== id) — the raw API/orderKey head, which could jump to an unexpected session that is neither the nearest visible row nor the latest. Extract pickNeighbourAfterRemoval(orderedList, id) (next row, or previous when the deleted row was last) and route topic deletion and both session paths through it, using each surface's display-ordered visible list (filteredGroupedSessions, which equals the full grouped list in modern mode). Both surfaces now follow one explicit "next visible row" policy. Adds helper unit tests and a modern-sidebar delete test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
There was a problem hiding this comment.
This review was translated automatically.
Findings:
-
Important:
src/renderer/utils/sidebar.ts:270/src/shared/data/preference/preferenceSchemas.ts:482changeui.sidebar.favoritesfrom a bare app-id array to taggedSidebarFavoriteItem[], but existing preference DB rows are loaded as-is and the seeder only inserts missing rows. A saved value like['assistants','agents','translate','mini_app']now reachesnormalizeSidebarFavoriteItem, has notype, is dropped, and the sidebar falls back to only the requiredassistantsitem. Please add a one-time preference data migration or storage-boundary normalization that rewrites legacy strings to{ type: 'app', id }, and cover this existing-row case in tests. -
Important:
src/renderer/components/chat/resourceList/AssistantResourceList.tsx:440andsrc/renderer/components/chat/resourceList/AgentResourceList.tsx:276consumeresourceMenuItemsfor the new options menu but no longer pass them through toResourceEntityRail.ResourceEntityRailstill relies on activeresourceMenuItemsto clear row selection and disable the selected-row repeat-click path (ResourceEntityRail.tsx:144). In classic layout, opening the Assistant/Agent/Skill resource catalog leaves the current rail entity visually selected, and clicking that selected row runsonSelected*Clickinstead of normal selection, so it toggles the right resource pane rather than closing the catalog and returning to the conversation. Please forward the menu state, or otherwise give the rail the same active-resource-view signal, so active catalog views suppress entity selection/repeat-click behavior. -
Important:
src/renderer/pages/home/HomePage.tsx:523can fall back into a draft for the assistant that was just deleted.Topics.tsx:804callsdeleteAssistant(assistantId, { deleteTopics: true }), then invokesonActiveAssistantDeletedbefore refreshing assistants/topics. When there is no remaining topic to select,handleActiveAssistantDeletedcallsstartDraftAssistantSelection()with no payload;resolveDraftAssistantTargetstill sees the stale assistant list and stalelastUsedAssistantId, so it can resolve back todeletedAssistantId. The UI can then show a draft bound to a non-existent assistant, and creating/sending from it can fail. Please make this fallback exclude the deleted assistant or refresh/update the assistant source before starting the fallback draft, and cover the "delete active assistant with no remaining topics" case. -
Important:
src/renderer/pages/agents/AgentPage.tsx:126auto-opens the classic Agent session pane on every fresh mount when the stored display mode isagentand the session position isright. The effect always callssetSessionPaneOpen(true)after remount, so a user who manually closes the right session pane hasui.agent.right_pane_open=falseoverwritten the next time the Agent page/app is re-entered. Please preserve explicit storedfalsevalues and only default-open when the setting is genuinely unset/being initialized; add a remount/restart-style test for the manual-close case. -
Important:
src/renderer/i18n/locales/en-us.json:1099and the matchingzh-cn/zh-twentries regress the assistant clear flow back to "topics" while the surrounding UI labels it as conversations. The dialog title remains "Clear conversations", but the content/menu/success copy now says "topics", producing mixed user-visible terminology for the same action. Please keep these strings consistent with the conversation wording, e.g. "Clear conversations", "Clearing conversations will delete…", and "Cleared {{count}} conversations" with matching Chinese translations.
Manual intervention:
- Required: Preference/data migration and chat/agent UI runtime behavior changed; please manually verify upgraded sidebar favorites, classic resource rail/catalog switching, active-assistant deletion fallback, and right-position Agent session pane close persistence.
Verification:
- Reviewed PR #16739 at head
98d4935806c6a355bdfa2162cc7e93fb7024ce48in the pinned worktree. - Checked existing reviews/comments and did not repeat previously addressed feedback.
- Checked GitHub CI via
gh; current checks are passing or skipped. - Did not run local
pnpm lint/test/format, per repository PR review rules.
After the fix, please request eeee0717 for further review.
Original Content
Findings:
-
Important:
src/renderer/utils/sidebar.ts:270/src/shared/data/preference/preferenceSchemas.ts:482changeui.sidebar.favoritesfrom a bare app-id array to taggedSidebarFavoriteItem[], but existing preference DB rows are loaded as-is and the seeder only inserts missing rows. A saved value like['assistants','agents','translate','mini_app']now reachesnormalizeSidebarFavoriteItem, has notype, is dropped, and the sidebar falls back to only the requiredassistantsitem. Please add a one-time preference data migration or storage-boundary normalization that rewrites legacy strings to{ type: 'app', id }, and cover this existing-row case in tests. -
Important:
src/renderer/components/chat/resourceList/AssistantResourceList.tsx:440andsrc/renderer/components/chat/resourceList/AgentResourceList.tsx:276consumeresourceMenuItemsfor the new options menu but no longer pass them through toResourceEntityRail.ResourceEntityRailstill relies on activeresourceMenuItemsto clear row selection and disable the selected-row repeat-click path (ResourceEntityRail.tsx:144). In classic layout, opening the Assistant/Agent/Skill resource catalog leaves the current rail entity visually selected, and clicking that selected row runsonSelected*Clickinstead of normal selection, so it toggles the right resource pane rather than closing the catalog and returning to the conversation. Please forward the menu state, or otherwise give the rail the same active-resource-view signal, so active catalog views suppress entity selection/repeat-click behavior. -
Important:
src/renderer/pages/home/HomePage.tsx:523can fall back into a draft for the assistant that was just deleted.Topics.tsx:804callsdeleteAssistant(assistantId, { deleteTopics: true }), then invokesonActiveAssistantDeletedbefore refreshing assistants/topics. When there is no remaining topic to select,handleActiveAssistantDeletedcallsstartDraftAssistantSelection()with no payload;resolveDraftAssistantTargetstill sees the stale assistant list and stalelastUsedAssistantId, so it can resolve back todeletedAssistantId. The UI can then show a draft bound to a non-existent assistant, and creating/sending from it can fail. Please make this fallback exclude the deleted assistant or refresh/update the assistant source before starting the fallback draft, and cover the "delete active assistant with no remaining topics" case. -
Important:
src/renderer/pages/agents/AgentPage.tsx:126auto-opens the classic Agent session pane on every fresh mount when the stored display mode isagentand the session position isright. The effect always callssetSessionPaneOpen(true)after remount, so a user who manually closes the right session pane hasui.agent.right_pane_open=falseoverwritten the next time the Agent page/app is re-entered. Please preserve explicit storedfalsevalues and only default-open when the setting is genuinely unset/being initialized; add a remount/restart-style test for the manual-close case. -
Important:
src/renderer/i18n/locales/en-us.json:1099and the matchingzh-cn/zh-twentries regress the assistant clear flow back to "topics" while the surrounding UI labels it as conversations. The dialog title remains "Clear conversations", but the content/menu/success copy now says "topics", producing mixed user-visible terminology for the same action. Please keep these strings consistent with the conversation wording, e.g. "Clear conversations", "Clearing conversations will delete…", and "Cleared {{count}} conversations" with matching Chinese translations.
Manual intervention:
- 需要:Preference/data migration and chat/agent UI runtime behavior changed; please manually verify upgraded sidebar favorites, classic resource rail/catalog switching, active-assistant deletion fallback, and right-position Agent session pane close persistence.
Verification:
- Reviewed PR #16739 at head
98d4935806c6a355bdfa2162cc7e93fb7024ce48in the pinned worktree. - Checked existing reviews/comments and did not repeat previously addressed feedback.
- Checked GitHub CI via
gh; current checks are passing or skipped. - Did not run local
pnpm lint/test/format, per repository PR review rules.
修复后请 request eeee0717 再进行后续 review。
Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
Signed-off-by: jd <59188306+zhangjiadi225@users.noreply.github.com>
Signed-off-by: kangfenmao <kangfenmao@qq.com>
Signed-off-by: kangfenmao <kangfenmao@qq.com>
@eeee0717 has re-checked and handled these items:
Synced with latest Original Content@eeee0717 已对这几项重新检查并处理:
已同步最新 |
…e resource menu items
…w-switching Signed-off-by: kangfenmao <kangfenmao@qq.com> # Conflicts: # src/renderer/pages/agents/AgentPage.tsx # src/renderer/pages/agents/__tests__/AgentPage.test.tsx # src/renderer/pages/agents/components/AgentRightPane/AgentRightPane.tsx # src/renderer/pages/home/HomePage.tsx # src/renderer/pages/home/__tests__/HomePage.test.tsx # v2-refactor-temp/docs/chat/chat-layout-modes.md
0xfullex
left a comment
There was a problem hiding this comment.
Reviewed the latest head and left two inline comments: one Home classic topic pane persistence regression, and one upgrade-cache default normalization issue.
|
|
||
| if (hasAutoOpenedClassicTopicPaneRef.current) return | ||
| hasAutoOpenedClassicTopicPaneRef.current = true | ||
| setTopicPaneOpen(true) |
There was a problem hiding this comment.
This review comment was translated automatically.
[A1] The Home classic topic pane writes ui.chat.right_pane_open back to true every time the page is re-entered. If the user manually closes the right topic pane when topic.tab.position = right, the next time they re-enter Home/app, this effect will overwrite the saved false, causing the panel to automatically open again. The Agent side has already been changed to only force-write when explicitly switching to the right; it is also recommended here to remove the mount auto-open and use setTopicPaneOpen(position === "right", { force: true }) in setTopicListPosition to handle explicit position switching.
Original Content
[A1] Home classic topic pane 在每次重新进入页面时都会把 ui.chat.right_pane_open 写回 true。如果用户在 topic.tab.position = right 时手动关闭右侧 topic pane,下一次重新进入 Home/app 时这个 effect 会覆盖已保存的 false,导致面板又自动打开。Agent 侧已经改为只在显式切换到右侧时 force 写入;这里也建议移除 mount auto-open,并在 setTopicListPosition 里用 setTopicPaneOpen(position === "right", { force: true }) 处理显式位置切换。
There was a problem hiding this comment.
This review comment was translated automatically.
This is expected behavior: when the topic position is switched to the right, the right topic panel needs to be opened by default, otherwise users won't see the topic list after switching.
Original Content
这里是预期行为:话题位置切换到右侧时,需要默认打开右侧话题面板,否则用户切换后看不到话题列表。
There was a problem hiding this comment.
This review comment was translated automatically.
I agree here that "explicitly switching to the right should open". The problem is the current effect doesn't only respond to explicit switching - it also executes when Home remounts or the app re-enters, so the user's manually saved ui.chat.right_pane_open=false gets overwritten.
You can keep the switching behavior, but move the open action to setTopicListPosition, e.g., setTopicPaneOpen(position === "right", { force: true }); this way switching from left/time to right still opens the list, but re-entering the page respects the user's last manually closed state.
Original Content
这里我同意“显式切换到右侧时应该打开”。问题是当前 effect 不只响应显式切换,它在 Home 重新 mount / app 重新进入时也会执行,所以用户手动关闭后保存的 ui.chat.right_pane_open=false 会被覆盖。
可以保留切换行为,但把打开动作放到 setTopicListPosition 里,例如 setTopicPaneOpen(position === "right", { force: true });这样从 left/time 切到 right 时仍会打开列表,但重新进入页面时会尊重用户上次手动关闭的状态。
There was a problem hiding this comment.
This review comment was translated automatically.
Adjusted in this direction: removed the effect that automatically writes the open state when Home remounts, only opening/closing the right topic panel when explicitly switching topic positions via setTopicPaneOpen(position === "right", { force: true }). This way, switching to the right will still open by default, and re-entering the page will respect the user's last manual close state. Added corresponding tests.
Original Content
已按这个方向调整:移除了 Home 重新 mount 时自动写入打开状态的 effect,只在显式切换话题位置时通过 setTopicPaneOpen(position === "right", { force: true }) 打开/关闭右侧话题面板。这样切到右侧仍会默认打开,重新进入页面会尊重用户上次手动关闭的状态。已补充对应测试。
| 'ui.chat.right_pane_open': false, | ||
| 'ui.topic.expansion.time': [], | ||
| 'ui.topic.expansion.assistant': [], | ||
| 'ui.topic.expansion.assistant': null, |
There was a problem hiding this comment.
This review comment was translated automatically.
[B7] Changing the default value to null here only affects new users who don't have old persist cache. The old version has already written the default [] to cs_cache_persist, and the renderer loadPersistCache will first set the new default value, then unconditionally overwrite it with the old value from localStorage; upgraded users will therefore still get [], and resolveDefaultCollapsedGroupIds() will not enter the "no preference written" default collapse logic. It is recommended to normalize these legacy [] to null at the persist-cache load boundary, or use a versioned/new key, and add an upgrade test case for the old cs_cache_persist.
Original Content
[B7] 这里把默认值改成 null 只会影响没有旧 persist cache 的新用户。旧版本已经把默认 [] 写入 cs_cache_persist,而 renderer loadPersistCache 会先设新默认值、再无条件用 localStorage 里的旧值覆盖;升级用户因此仍会拿到 [],resolveDefaultCollapsedGroupIds() 不会进入“未写入偏好”的默认折叠逻辑。建议在 persist-cache 加载边界把这几个 legacy [] 规范化为 null,或使用 versioned/new key,并补一个旧 cs_cache_persist 的升级用例。
There was a problem hiding this comment.
This review comment was translated automatically.
This point will not be handled for now. This configuration is still in the testing phase, and we will not consider the upgrade compatibility migration from the existing [] in the old version of cs_cache_persist for now.
Original Content
这个点目前先不处理。该配置还在测试阶段,暂时不考虑旧版 cs_cache_persist 中已有 [] 的升级兼容迁移。
Signed-off-by: kangfenmao <kangfenmao@qq.com>
What this PR does
Before this PR:
Session/topic view switching and resource actions were spread across settings, sidebar behavior, and grouped menu surfaces. Agent and assistant resource controls also had inconsistent labels and nested edit-dialog tool tabs.
After this PR:
Session/topic display mode controls live with the relevant resource menus, legacy layout preferences are removed, resource list interactions are simplified, and Agent/Assistant edit tool tabs are shown directly. The related i18n strings, docs, preference schema, and tests are updated.
Fixes #N/A
Why we need it and why it was done in this way
The following tradeoffs were made:
The change keeps the behavior scoped to the existing chat and agent resource surfaces instead of adding another settings-level abstraction.
The following alternatives were considered:
Keeping the old settings switches was considered, but that would preserve duplicated layout controls and make the resource menu behavior harder to reason about.
Links to places where the discussion took place: N/A
Breaking changes
N/A
Special notes for your reviewer
This PR includes UI behavior changes in chat resource lists, AgentPage/HomePage display mode handling, and Agent/Assistant edit dialog tab presentation.
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
mainfor active development,v1for v1 maintenance fixes/gh-pr-review,gh pr diff, or GitHub UI) before requesting review from othersRelease note