feat(conversation-list): 添加置顶和取消置顶会话功能#434
feat(conversation-list): 添加置顶和取消置顶会话功能#4341024570189 wants to merge 1 commit intolexmin0412:mainfrom
Conversation
dosubot
Bot
added
the
size:L
|
Someone is attempting to deploy a commit to the lexmin0412's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughEnhanced the conversation list component with pinning functionality. Added local state for managing top/pinned conversations, updated the label type definition to support ReactNode, implemented top/untop conversation handlers, and refactored rendering to display pinned conversations separately from regular ones. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
我想增加一个左侧聊天列表的一个置顶功能，希望可以作为参考增加功能，或者看一眼我的代码 |
dosubot
Bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-app/src/components/conversation-list/index.tsx (1)
78-79: Remove duplicate validateFields call.Line 78 calls
validateFields()but doesn't use the result. Line 79 calls it again and uses the result. This is redundant.Apply this diff:
onOk: async () => { - await renameForm.validateFields() const values = await renameForm.validateFields() await renameConversationPromise(conversation.key, values.name) message.success('对话重命名成功') },
🧹 Nitpick comments (4)
packages/react-app/src/components/conversation-list/index.tsx (4)
1-1: Consider more semantically appropriate icons for pin/unpin actions.DownloadOutlined and UploadOutlined may not clearly convey "pin" and "unpin" operations to users. Consider using PushpinOutlined/PushpinFilled or similar icons that better represent pinning functionality.
114-129: Consider defining menu action keys as constants.Menu keys like 'delete', 'rename', 'top', 'untop' are used as magic strings across switch statement and menu items. Extracting them as constants improves maintainability and prevents typos.
const MENU_ACTIONS = { DELETE: 'delete', RENAME: 'rename', TOP: 'top', UNTOP: 'untop', } as constThen use
MENU_ACTIONS.DELETEetc. in the switch statement and menu items.Also applies to: 136-175
200-200: Optimize filtering logic for better performance.The nested filter with
.some()has O(n*m) time complexity. For large conversation lists, this could cause performance issues on every render.Use a Set for O(n) lookup:
+const topItemKeys = new Set(topItems.map(item => item.key)) +const normalItems = items.filter(item => !topItemKeys.has(item.key)) + {/* 普通会话列表 */} <Conversations className="!p-0" - items={items.filter(item => !topItems.some(topItem => topItem.key === item.key))} + items={normalItems} activeKey={activeKey} onActiveChange={onActiveChange} menu={generateMenuConfig(false)} />
180-195: Simplify conditional rendering.Using both
display: 'none'and checkingtopItems.length > 0for visibility is redundant. Use conditional rendering instead for cleaner code.-{/* 置顶列表 */} -<div - style={{ - marginBottom: '10px', - borderBottom: topItems.length > 0 ? '1px solid #f0f0f0' : 'none', - paddingBottom: '10px', - display: topItems.length > 0 ? 'block' : 'none', - }} -> - <Conversations - className="!p-0" - items={topItems} - activeKey={activeKey} - onActiveChange={onActiveChange} - menu={generateMenuConfig(true)} - /> -</div> +{/* 置顶列表 */} +{topItems.length > 0 && ( + <div + style={{ + marginBottom: '10px', + borderBottom: '1px solid #f0f0f0', + paddingBottom: '10px', + }} + > + <Conversations + className="!p-0" + items={topItems} + activeKey={activeKey} + onActiveChange={onActiveChange} + menu={generateMenuConfig(true)} + /> + </div> +)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-app/src/components/conversation-list/index.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/{api,core,docs,helpers,platform,react-app,theme}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not import the deprecated packages/components library from any other subpackage
Files:
packages/react-app/src/components/conversation-list/index.tsx
🔇 Additional comments (2)
packages/react-app/src/components/conversation-list/index.tsx (2)
8-8: Type change from string to ReactNode requires verification of rename function compatibility.Changing
labeltoReactNodemay conflict with downstream usage if it's cast to a string elsewhere. Verify that the rename function at line 63 and other usages properly handle ReactNode values, or consider using a separate field for rendering iflabelneeds to remain a string identifier.
58-61: Guard condition may indicate incomplete data validation upstream.If
conversation.labelcan be empty, determine whether this represents legitimate data that should be prevented earlier (e.g., at creation time) or if it's defensive programming for unexpected edge cases. If labels must always be populated when conversations are created, move this validation to the data source. If defensive programming is intentional, consider clarifying the guard's purpose in a comment.
| props | ||
|
|
||
| const [renameForm] = Form.useForm() | ||
| const [topItems, setTopItems] = useState<IConversationItem[]>([]) |
There was a problem hiding this comment.
Pinned state is not persisted and will be lost.
The topItems state is local and ephemeral - it will reset on component unmount or page refresh. Users will lose their pinning preferences, which severely impacts UX for a pinning feature.
Consider:
- Lift state to parent component and accept
pinnedItemsas a prop with anonPinnedChangecallback, or - Persist to localStorage/backend, or
- Add a prop like
topConversationPromise: (conversationId: string) => Promise<unknown>to persist pin state
Apply this diff to lift state to parent:
interface IConversationListProps {
deleteConversationPromise: (conversationId: string) => Promise<unknown>
renameConversationPromise: (conversationId: string, name: string) => Promise<unknown>
+ topConversationPromise?: (conversationId: string, isPinned: boolean) => Promise<unknown>
items: IConversationItem[]
+ pinnedConversationIds?: string[]
activeKey?: string
onActiveChange?: (key: string) => void
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/react-app/src/components/conversation-list/index.tsx around line 42,
the local topItems state is ephemeral and causes pinned items to be lost on
unmount/refresh; replace the internal useState with a controlled prop (e.g.,
pinnedItems: IConversationItem[]) and an onPinnedChange callback so the parent
owns persistence and updates; update the component props/type definitions to
accept pinnedItems and onPinnedChange (default pinnedItems to []), remove
setTopItems usage and call onPinnedChange whenever a pin/unpin occurs;
alternatively support a persistence callback prop like
topConversationPromise(conversationId) to persist pin state to
localStorage/backend if lifting is not desirable.
| const validConversation: IConversationItem = { | ||
| key: conversation.key, | ||
| label: conversation.label || '未命名会话', | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated validConversation construction.
The same object construction pattern appears twice (lines 109-112 and 168-171). Extract this into a helper function to reduce duplication.
Add this helper before handleMenuClick:
+const ensureValidConversation = (conversation: IConversationItem): IConversationItem => ({
+ key: conversation.key,
+ label: conversation.label || '未命名会话',
+})
+
const handleMenuClick = (conversation: IConversationItem, menuKey: string) => {
- const validConversation: IConversationItem = {
- key: conversation.key,
- label: conversation.label || '未命名会话',
- }
+ const validConversation = ensureValidConversation(conversation)And use it in generateMenuConfig:
onClick: async (menuInfo: { domEvent: React.MouseEvent; key: string }) => {
menuInfo.domEvent.stopPropagation()
- const validConversation: IConversationItem = {
- key: conversation.key,
- label: conversation.label || '未命名会话',
- }
+ const validConversation = ensureValidConversation(conversation)
handleMenuClick(validConversation, menuInfo.key)
},Also applies to: 168-171
🤖 Prompt for AI Agents
In packages/react-app/src/components/conversation-list/index.tsx around lines
109-112 and 168-171, the construction of validConversation is duplicated; create
a small helper (e.g., createValidConversationItem(conversation: IConversation):
IConversationItem) placed before handleMenuClick that returns { key:
conversation.key, label: conversation.label || '未命名会话' }, then replace the
inline object constructions in both locations (lines ~109-112 and ~168-171) to
call this helper so the logic is centralized and duplication removed.
1 participant
Description
Linked Issues
Additional context
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.