-
Notifications
You must be signed in to change notification settings - Fork 2.7k
refactor(MiniWindow): separate network request codes into independent hook #8249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR extracts the network request and streaming logic from the HomeWindow
component into a standalone useSendMessage
hook, simplifying the component and centralizing messaging behavior.
- Create
useSendMessage
hook inuseMiniWindow.ts
- Replace inline messaging logic in
HomeWindow.tsx
with calls to the new hook - Update imports and callbacks (
sendMessage
,pause
,reset
) inHomeWindow
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
src/renderer/src/windows/mini/home/HomeWindow.tsx |
Removed inline network/store logic and integrated useSendMessage |
src/renderer/src/hooks/useMiniWindow.ts |
Introduced useSendMessage hook with sendMessage , pause , reset |
Comments suppressed due to low confidence (2)
src/renderer/src/hooks/useMiniWindow.ts:22
- [nitpick] The file is named
useMiniWindow.ts
but exportsuseSendMessage
. Consider renaming the file touseSendMessage.ts
(or renaming the hook) to keep the file name and exported hook name consistent.
export const useSendMessage = (options: SendMessageOptions) => {
src/renderer/src/hooks/useMiniWindow.ts:1
- The new
useSendMessage
hook contains complex logic around streaming, cancellation, and state management but lacks corresponding unit tests. Adding tests forsendMessage
,pause
, andreset
would help ensure behavior remains correct.
import { fetchChatCompletion } from '@renderer/services/ApiService'
{ | ||
setIsOutputted(true) | ||
if (thinkingBlockId) { | ||
store.dispatch( | ||
updateOneBlock({ id: thinkingBlockId, changes: { status: MessageBlockStatus.STREAMING } }) | ||
) | ||
} else { | ||
const block = createThinkingBlock(assistantMessage.id, '', { | ||
status: MessageBlockStatus.STREAMING | ||
}) | ||
thinkingBlockId = block.id | ||
store.dispatch( | ||
newMessagesActions.updateMessage({ | ||
topicId, | ||
messageId: assistantMessage.id, | ||
updates: { blockInstruction: { id: block.id } } | ||
}) | ||
) | ||
store.dispatch(upsertOneBlock(block)) | ||
} | ||
} | ||
break | ||
case ChunkType.THINKING_DELTA: | ||
{ | ||
setIsOutputted(true) | ||
if (thinkingBlockId) { | ||
throttledBlockUpdate(thinkingBlockId, { | ||
content: chunk.text, | ||
thinking_millsec: chunk.thinking_millsec | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The onChunkReceived
callback is very large and deeply nested. Extract individual chunk handlers into separate helper functions to improve readability and make unit testing easier.
{ | |
setIsOutputted(true) | |
if (thinkingBlockId) { | |
store.dispatch( | |
updateOneBlock({ id: thinkingBlockId, changes: { status: MessageBlockStatus.STREAMING } }) | |
) | |
} else { | |
const block = createThinkingBlock(assistantMessage.id, '', { | |
status: MessageBlockStatus.STREAMING | |
}) | |
thinkingBlockId = block.id | |
store.dispatch( | |
newMessagesActions.updateMessage({ | |
topicId, | |
messageId: assistantMessage.id, | |
updates: { blockInstruction: { id: block.id } } | |
}) | |
) | |
store.dispatch(upsertOneBlock(block)) | |
} | |
} | |
break | |
case ChunkType.THINKING_DELTA: | |
{ | |
setIsOutputted(true) | |
if (thinkingBlockId) { | |
throttledBlockUpdate(thinkingBlockId, { | |
content: chunk.text, | |
thinking_millsec: chunk.thinking_millsec | |
}) | |
} | |
} | |
handleThinkingStart(chunk, { | |
setIsOutputted, | |
thinkingBlockId, | |
assistantMessage, | |
topicId | |
}) | |
break | |
case ChunkType.THINKING_DELTA: | |
handleThinkingDelta(chunk, { | |
setIsOutputted, | |
thinkingBlockId | |
}) |
Copilot uses AI. Check for mistakes.
isLoading, | ||
isOutputted, | ||
error, | ||
setError, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Exposing the raw setError
setter allows external components to arbitrarily modify the error state. Consider providing a clearError
function instead to encapsulate error management.
setError, | |
clearError, |
Copilot uses AI. Check for mistakes.
setIsOutputted(false) | ||
setError(null) | ||
|
||
await fetchChatCompletion({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Multiple individual Redux dispatches occur for each streaming chunk. Consider batching related dispatches or using unstable_batchedUpdates
to reduce re-renders and improve performance.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sendMessage的操作(如fetchChatCompletion)拆出来成为独立方法是对的,但是同时将其和耦合成hook,未必是好的设计
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isLoading
、isOutputted
、error
这三个状态的更新函数目前仅有 setError
需要在组件副作用中调用(并且可以在进一步重构中去除),而其他函数与组件没有关联,将它们与网络请求封装到 hook 中,对外暴露只读的状态,这与 useSwr
或 useRequest
的思路类似,我觉得是个合理的方案。
另一方面,上述三个状态与网络请求是相关的,需要在请求流程内更新其值。如果不封装为 hook,那么需要调用者提供方法来更新状态,例如传入 beforeRequest
、afterRequest
等回调,我并不觉得这种方式会更合理。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
既然fetchChatCompletion不依赖组件的状态更新,那fetchChatCompletion可以和组件完全脱钩,不使用任何use,也避免了组件状态的耦合和fetchChatCompletion的影响。
可以单独将fetchChatCompletion拆出来即可,这也是最重的一个方法
What this PR does
In file
HomeWindow.tsx
, UI state, event callbacks, and network request codes are mixed together, making the component very lengthy. The network request part is relatively independent, and separating it into a separate file will make the logic of each part clearer.Why we need it and why it was done in this way
As mentioned above.
Breaking changes
Nope.
Special notes for your reviewer
Nope.
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.
Release note