refactor(core): consolidate chat session commands under /session#386
refactor(core): consolidate chat session commands under /session#386huangdijia wants to merge 2 commits intochenhg5:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR consolidates chat session management commands under a canonical /session <subcommand> namespace (while keeping existing top-level commands working), and updates help/cards/docs accordingly.
Changes:
- Add
/sessioncommand routing in the core engine, dispatching to existing session commands and handling subcommand aliases (name→rename,rm/del→delete). - Update help cards, list/delete card actions, and i18n strings to prefer
/session ...flows. - Update English/Chinese docs plus add regression tests for
/sessiondispatch, aliases, disabled-command policy behavior, and card actions.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates documented session commands to /session .... |
| README.zh-CN.md | Same documentation update in Chinese. |
| docs/usage.md | Updates usage guide session command table to /session .... |
| docs/usage.zh-CN.md | Same usage guide update in Chinese. |
| core/i18n.go | Updates help/hints/usage text to reference /session ... paths. |
| core/engine.go | Adds /session command handling + routes card navigation/actions through /session .... |
| core/engine_test.go | Adds tests covering /session dispatch, aliases, policy blocking, and card action updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(parts) > 1 { | ||
| descKey = parts[1] | ||
| } | ||
| } |
There was a problem hiding this comment.
renderHelpGroupCard derives the i18n description key from the second token of /session ... commands (e.g. rename). There is no rename entry in core/i18n.go (builtin descriptions use name), so the help card will render the description as the raw string rename instead of a localized description. Consider mapping rename → name here (or adding a dedicated rename message key) so /session rename shows the same localized description as the existing /name command.
| } | |
| } | |
| // Normalize subcommand keys that share descriptions with existing commands. | |
| // For example, `/session rename` should use the same description as `/name`. | |
| if descKey == "rename" { | |
| descKey = "name" | |
| } |
| }{ | ||
| {[]string{"session"}, "session"}, | ||
| {[]string{"new"}, "new"}, | ||
| {[]string{"list", "sessions"}, "list"}, |
There was a problem hiding this comment.
Adding the new /session builtin command makes prefix matching ambiguous with the existing /sessions alias for list (and any other se... aliases), because matchPrefix treats aliases and canonical names equally and returns "" on ambiguity. This means prefixes that previously worked (e.g. /ses, /sess → /sessions → list) will now fail to resolve. If prefix matching compatibility matters, consider removing/renaming the sessions alias, or changing matchPrefix to prefer canonical names over aliases when resolving prefixes.
| {[]string{"list", "sessions"}, "list"}, | |
| {[]string{"list"}, "list"}, |
| MsgSearchHint: { | ||
| LangEnglish: "Use /switch <id> to switch to a session.", | ||
| LangChinese: "使用 /switch <id> 切换到对应会话。", | ||
| LangTraditionalChinese: "使用 /switch <id> 切換到對應會話。", | ||
| LangJapanese: "/switch <id> でセッションを切り替え。", | ||
| LangSpanish: "Usa /switch <id> para cambiar a una sesión.", | ||
| LangEnglish: "Use /session switch <id> to switch to a session.", | ||
| LangChinese: "使用 /session switch <id> 切换到对应会话。", | ||
| LangTraditionalChinese: "使用 /session switch <id> 切換到對應會話。", | ||
| LangJapanese: "/session switch <id> でセッションを切り替え。", | ||
| LangSpanish: "Usa /session switch <id> para cambiar a una sesión.", |
There was a problem hiding this comment.
MsgSearchHint was updated to point users to /session switch ..., but MsgSearchUsage (shown when /session search is invoked without args) still says Usage: /search <keyword>. That makes the UX inconsistent with the new “/session … is canonical” documentation. Consider updating MsgSearchUsage to mention /session search (optionally also noting /search remains a compatible alias).
| e.cmdList(p, msg, subArgs) | ||
| case "search": | ||
| e.cmdSearch(p, msg, subArgs) | ||
| case "switch": | ||
| e.cmdSwitch(p, msg, subArgs) | ||
| case "rename": |
There was a problem hiding this comment.
/session search and /session switch delegate to cmdSearch/cmdSwitch, but their usage/help strings still reference the legacy top-level commands (Usage: /search ... via MsgSearchUsage, and the hardcoded Usage: /switch ...). When users run /session search or /session switch incorrectly (e.g. missing args), the reply will contradict the new “/session … is canonical” docs. Consider updating those usage messages to mention /session search and /session switch (optionally keeping the old forms as aliases).
chenhg5
left a comment
chenhg5
left a comment
There was a problem hiding this comment.
LGTM. Good UX consolidation.
Review summary:
- ✅
/sessionas canonical namespace for session management - ✅ Backward compatible with existing top-level commands
- ✅ i18n strings updated
- ✅ Card navigation updated
- ✅ Tests added
- ✅ CI passes
Clean organization of session-related commands.
Summary
/sessionas the canonical chat command namespace for session management/session ...while keeping old top-level commands compatibleBackground
/new,/list,/switch, etc.)/session [sub-cmd]without breaking existing flowsChanges
cmdSessionand/sessioncommand registration in the engine/session new|list|search|switch|rename|current|history|deleteplus alias handling forname,del, andrm/session ...semantics/sessiondispatch, aliases, help rendering, and card actionsTest Plan
go test ./core -run 'TestCmdSession|TestCmdSwitch|TestRenderHelpCard|TestHandleCardNav_HelpSwitchesTabs|TestRenderListCard_MakesEveryVisibleSessionClickable|TestDeleteMode_CancelReturnsListCard|TestExecuteCardAction_(NewCleansUpAndCreatesSession|SessionNewDelegatesToNew)'go test ./...(currently fails on unrelated existing tests inagent/acp,core, andplatform/wecom)Risks
/session, so very short prefixes like/sesare more ambiguous than before/sessionrouting, so session card regressions would show up in help/list/delete flows first