Refactor: API Handling and Improve Type Safety in Frontend Components #2436
Conversation
WalkthroughThe changes update TypeScript configuration to target ES2023, add module declarations for 'lucide-react' and 'mermaid', and exclude 'mermaid' from the Electron renderer's DLL build. The codebase standardizes HTTP requests to use Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thanks for your contribution! I suggest fix one single problem in one PR, and please elaborate why you do this changes, thanks! |
|
You're welcome, sure thing please let me know which specific problem. |
|
|
||
| const EXCLUDE_MODULES = new Set([ | ||
| '@modelcontextprotocol/sdk', // avoid `Package path . is not exported from package` error | ||
| 'mermaid', |
There was a problem hiding this comment.
why exclude this module, what error you've met?
| if (isValidElement(children)) { | ||
| target = cloneElement(children, { | ||
| onClick: (e) => { | ||
| if (children.props.onClick) children.props.onClick(e) | ||
| setOpened(true) | ||
| const elementWithOnClick = children as React.ReactElement<{ onClick?: (e: React.MouseEvent) => void }>; | ||
|
|
||
| target = cloneElement(elementWithOnClick, { | ||
| onClick: (e: React.MouseEvent) => { | ||
| if (elementWithOnClick.props.onClick) elementWithOnClick.props.onClick(e); | ||
| setOpened(true); |
There was a problem hiding this comment.
I think we can make this simpler
| if (isValidElement(children)) { | |
| target = cloneElement(children, { | |
| onClick: (e) => { | |
| if (children.props.onClick) children.props.onClick(e) | |
| setOpened(true) | |
| const elementWithOnClick = children as React.ReactElement<{ onClick?: (e: React.MouseEvent) => void }>; | |
| target = cloneElement(elementWithOnClick, { | |
| onClick: (e: React.MouseEvent) => { | |
| if (elementWithOnClick.props.onClick) elementWithOnClick.props.onClick(e); | |
| setOpened(true); | |
| if (isValidElement<{ onClick?: (e: React.MouseEvent) => void }>(children)) { | |
| target = cloneElement(children as ReactElement<{ onClick?: (e: React.MouseEvent) => void }>, { | |
| onClick: (e: React.MouseEvent) => { | |
| if (typeof children.props.onClick === 'function') { | |
| children.props.onClick(e) | |
| } | |
| setOpened(true) |
| setOpened(true); | ||
| }, | ||
| }) | ||
| }); |
| @@ -0,0 +1,2 @@ | |||
| declare module 'lucide-react'; | |||
| declare module 'mermaid'; No newline at end of file | |||
There was a problem hiding this comment.
Why need this type declaration, can you show what type error you are trying to fix?
| "target": "es2021", | ||
| "module": "commonjs", | ||
| "lib": ["dom", "es2021"], | ||
| "lib": ["dom", "es2023"], |
| } | ||
| const res = await ofetch<Response>(`${API_ORIGIN}/api/license/detail`, { | ||
| retry: 3, | ||
| const res = await afetch(`${API_ORIGIN}/api/license/detail`, { |
There was a problem hiding this comment.
I'm not sure what are the internal differences between afetch and oftech, need to check carefully.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/renderer/components/SimpleSelect.tsx (1)
30-30: Fix type safety issue in onChange handler.The onChange handler casts
e.target.valuedirectly toTbut the Props interface now allowsT | undefined. This creates a type mismatch where undefined values could be passed but aren't properly handled.- onChange={(e) => props.onChange(e.target.value as T)} + onChange={(e) => props.onChange(e.target.value as T | undefined)}Or handle the undefined case explicitly:
- onChange={(e) => props.onChange(e.target.value as T)} + onChange={(e) => { + const value = e.target.value; + props.onChange(value === '' ? undefined : value as T); + }}src/renderer/packages/remote.ts (1)
90-99: Convert request body to JSON and include Content-Type header
afetchdelegates directly tofetchwithout serializing objects or setting headers. Passing a raw object will be coerced to"[object Object]"and omit the proper header. To keep consistency with other API calls, explicitly wrap the body inJSON.stringifyand add'Content-Type': 'application/json'.
- File:
src/renderer/packages/remote.ts, lines 90–99Suggested change:
const res = await afetch<Response>(`${API_ORIGIN}/chatbox_need_update/${version}`, { method: 'POST', retry: 3, - body: { - uuid: config.uuid, - os: os, - allowReportingAndTracking: settings.allowReportingAndTracking ? 1 : 0, - }, + headers: { + 'Content-Type': 'application/json', + }, + body: JSON.stringify({ + uuid: config.uuid, + os, + allowReportingAndTracking: settings.allowReportingAndTracking ? 1 : 0, + }), })
♻️ Duplicate comments (3)
.erb/configs/webpack.config.renderer.dev.dll.ts (1)
17-17: Add explanatory comment for mermaid exclusion.Following the pattern established for
@modelcontextprotocol/sdk, consider adding a comment explaining why mermaid needs to be excluded from the DLL bundle.'@modelcontextprotocol/sdk', // avoid `Package path . is not exported from package` error - 'mermaid', + 'mermaid', // [add reason for exclusion here]src/types/global.d.ts (1)
1-2: Module declarations prevent TypeScript import errors.These basic module declarations resolve TypeScript "Cannot find module" errors when importing lucide-react and mermaid. While minimal, they serve their purpose of allowing imports without compilation errors.
src/renderer/packages/remote.ts (1)
49-227: Address past reviewer concern about afetch vs ofetch differences.A previous reviewer (themez) expressed uncertainty about the differences between
afetchandofetch. Based on the code analysis,afetchprovides retry logic and error handling thatofetchmay not have had.The migration provides:
- Retry mechanism with configurable attempts
- Custom error handling for Chatbox API errors
- Consistent platform headers injection
- Network error handling with origin tracking
This represents a significant improvement in API reliability and error handling.
🧹 Nitpick comments (4)
src/renderer/packages/remote.ts (4)
156-163: Remove redundant method specification.The explicit
method: 'GET'is redundant since GET is the default HTTP method. Consider removing it for cleaner code.const res = await afetch(`${API_ORIGIN}/api/premium/price`, { - method: 'GET', }, { retry: 3, })
169-176: Remove redundant method specification.Same issue as with
getPremiumPrice- the explicitmethod: 'GET'is unnecessary.const res = await afetch(`${API_ORIGIN}/api/remote_config/${config}`, { - method: 'GET', }, { retry: 3, })
201-211: Remove redundant method specification.Another instance of unnecessary
method: 'GET'specification.const res = await afetch(`${API_ORIGIN}/api/license/detail`, { - method: 'GET', headers: { Authorization: params.licenseKey, }, }, {
217-227: Remove redundant method specification.Final instance of unnecessary
method: 'GET'specification.const res = await afetch(`${API_ORIGIN}/api/license/detail/realtime`, { - method: 'GET', headers: { Authorization: params.licenseKey, }, }, {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
release/app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.erb/configs/webpack.config.renderer.dev.dll.ts(1 hunks)src/renderer/components/PopoverConfirm.tsx(1 hunks)src/renderer/components/SimpleSelect.tsx(1 hunks)src/renderer/packages/remote.ts(7 hunks)src/types/global.d.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/renderer/packages/remote.ts (2)
src/renderer/packages/request.ts (1)
afetch(7-64)src/shared/types.ts (3)
CopilotDetail(474-484)RemoteConfig(497-500)ChatboxAILicenseDetail(502-516)
🔇 Additional comments (8)
tsconfig.json (1)
6-6: ES2023 lib update provides access to modern JavaScript features.This change enables ES2023 APIs in the type environment while maintaining ES2021 as the compilation target. This is a standard practice that allows developers to use modern JavaScript features during development while ensuring output compatibility.
src/renderer/components/PopoverConfirm.tsx (1)
32-39: Improved type safety and readability.The refactoring addresses previous feedback by using explicit typing with a named variable and removing the trailing comma. The code is now more readable and type-safe while maintaining the same functionality.
src/renderer/components/SimpleSelect.tsx (1)
6-8: Props interface correctly allows undefined values.The interface changes appropriately increase component flexibility by allowing undefined values for controlled components.
src/renderer/packages/remote.ts (5)
15-16: LGTM: Dependencies added for functionality used later.The new imports
uniqfor deduplication andplatformfor platform detection are appropriately added and used later in the code.
49-64: Good improvements with defensive programming.The migration to
afetchwith explicit JSON parsing and the null check forpoolat line 61 are good defensive programming practices. The use ofuniqprevents duplicate API origins.
126-137: Excellent type safety improvements.The explicit
CopilotListResponsetype definition and explicit JSON parsing improve type safety and make the code more maintainable.
140-146: Consistent JSON serialization implementation.Good use of
JSON.stringify()for the request body, which is consistent with the pattern established in other functions.
187-195: Consistent implementation with good type safety.The explicit type definition and JSON handling follow the established pattern. Well implemented.
2 participants
This PR includes multiple improvements and fixes across the codebase:
Refactor and Enhancement
Replaced all instances of ofetch with a more consistent and retryable afetch wrapper in remote.ts, improving error resilience and consistency in API handling.
Enhanced JSON parsing and type safety for responses like CopilotListResponse, PremiumPriceResponse, and others.
Improved null-checking logic when updating API origin pool to prevent unexpected behavior.
Component Fixes
PopoverConfirm.tsx: Refactored cloneElement logic to ensure type-safe event propagation without affecting existing styles or functionality.
SimpleSelect.tsx: Updated Props interface to allow undefined for value and onChange, enabling better flexibility in controlled components.
Configuration and Compatibility
Updated tsconfig.json to target "es2023" for modern JavaScript features.
Declared missing modules lucide-react and mermaid in global.d.ts to suppress TypeScript errors during builds.
Minor cleanup in .erb/configs/webpack.config.renderer.dev.dll.ts to better manage excluded modules.
Summary
This PR refactors the API layer, improves frontend type safety, updates tooling config, and ensures more stable builds with retryable fetch logic.
Summary by CodeRabbit
New Features
Refactor
Chores