feat: refactor http as a configurable service #834
Conversation
WalkthroughThe changes involve several modifications across multiple files, primarily enhancing the HTTP service functionality in the application. Key updates include the introduction of a new HTTP management module, adjustments to existing exports, and the removal of outdated configurations. The structure of the exported objects has been refined, and error handling mechanisms have been improved to provide a more robust user experience during API interactions. Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
github-actions
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: 6
🧹 Outside diff range and nitpick comments (8)
designer-demo/registry.js (3)
47-47: LGTM: Addition of http import and configurationThe addition of
httpimport from '@opentiny/tiny-engine' andhttpConfigfrom './src/http' aligns well with the PR's objective of making the HTTP service configurable. Separating the HTTP configuration into its own file is a good practice for modularity.Consider grouping related imports together for better readability. You could move the
httpConfigimport next to thehttpimport from '@opentiny/tiny-engine'.Also applies to: 50-50
Line range hint
55-55: LGTM: Updates to the exported configuration objectThe changes to the exported configuration object are well-aligned with the PR objectives:
- Addition of
GenerateCodeServiceto themetasarray in therootobject.- Addition of the
httpproperty with configuration options.These changes effectively integrate the new HTTP service configuration into the existing structure.
Consider adding a brief comment above the
httpproperty to explain its purpose and how it relates to the HTTP service configuration. This would improve the maintainability of the code for future developers.Also applies to: 106-107
Line range hint
1-108: Summary: Successful implementation of configurable HTTP serviceThis PR successfully achieves its objective of refactoring the HTTP service into a configurable service. The changes are well-integrated into the existing structure of the
registry.jsfile. Here's a summary of the key changes:
- Addition of
GenerateCodeServiceandhttpimports.- Import of
httpConfigfrom a local file.- Integration of
GenerateCodeServiceinto themetasarray.- Addition of the
httpproperty in the exported configuration object.These changes enhance the modularity and configurability of the HTTP service without disrupting the overall structure of the configuration file. The implementation appears to be non-breaking, as it adds new functionality without modifying existing behavior.
To further improve this implementation, consider:
- Adding documentation comments for the new
httpconfiguration to explain its usage and options.- If not already done, create unit tests for the new HTTP service configuration to ensure its correctness and prevent regressions.
- Update any relevant documentation or README files to reflect these new configuration options for the HTTP service.
packages/design-core/index.js (1)
41-41: Consider removing the blank line for consistency.The blank line after the new
httpexport (line 41) seems unnecessary and inconsistent with the file's structure. Other export statements are not separated by blank lines.For better consistency, consider removing this blank line:
export { default as http } from '@opentiny/tiny-engine-http' - export * from '@opentiny/tiny-engine-meta-register'designer-demo/src/http/index.js (4)
11-17: Localize Error Messages for InternationalizationThe
showErrorfunction displays error messages hardcoded in Chinese. To support a wider user base and facilitate future internationalization, consider using a localization framework or moving message strings to a resource file.You can refactor the code as follows to use a localization function:
const showError = (url, message) => { globalNotify({ type: 'error', title: '接口报错', - message: `报错接口: ${url} \n报错信息: ${message ?? ''}` + message: i18n.t('errorMessage', { url, message: message ?? '' }) }) }
55-56: Remove Commented-Out CodeThere's a commented-out line of code which can lead to confusion and clutter the codebase.
Apply this diff to remove the unnecessary line:
getMetaApi('engine.service.http').request(response.config).then(resolve, reject) - // http.request(response.config).then(resolve, reject)
83-85: Review Comment Language ConsistencyThe code contains comments in Chinese, such as
// 仅在本地开发时，启用 withCredentials. For consistency and broader understanding among global team members, consider converting comments to English.For example:
-// 仅在本地开发时，启用 withCredentials +// Enable withCredentials only during local development
100-100: UseconstInstead ofletfor Immutable VariablesThe
environmentvariable is not reassigned after its initial declaration. Usingconstmakes the intent clear that the variable should not change.Apply this diff to update the variable declaration:
-let environment = import.meta.env +const environment = import.meta.env
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- designer-demo/registry.js (2 hunks)
- designer-demo/src/http/index.js (1 hunks)
- packages/design-core/index.js (1 hunks)
- packages/http/index.js (0 hunks)
- packages/http/src/config.js (0 hunks)
- packages/http/src/index.js (2 hunks)
- packages/http/vite.config.js (1 hunks)
💤 Files with no reviewable changes (2)
- packages/http/index.js
- packages/http/src/config.js
🔇 Additional comments (10)
packages/http/vite.config.js (1)
33-33: Approved, but please verify the impact of CSS removal and document the change.The removal of the
banneroption from the Rollup output configuration aligns with the PR objective of refactoring the HTTP service. However, please address the following points:
Verify that removing the
import "./style.css"banner doesn't negatively impact the styling of the library. If the CSS is now handled differently or is no longer needed, ensure this is reflected in the documentation.Document the reason for this change in the PR description or in a comment. This will help future maintainers understand the rationale behind the removal of the CSS import.
To ensure no unintended consequences, please run the following script to check for any remaining references to
style.cssin the package:This script will help verify that the removal of the CSS import was intentional and comprehensive.
✅ Verification successful
To ensure all references to
style.cssare accounted for, please run the following updated script:
Verified: Removal of CSS import is safe
The removal of the
banneroption andimport "./style.css"in the Rollup output configuration has been verified. There are no remaining references tostyle.cssin thepackages/httppackage, and thestyle.cssfile does not exist. This change aligns with the PR objective of refactoring the HTTP service and does not introduce any unintended side effects.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for references to style.css in the http package # Search for style.css references echo "Searching for style.css references:" rg --type js --type vue "style\.css" packages/http # Check if style.css file exists echo "Checking if style.css file exists:" fd -t f "style.css" packages/httpLength of output: 274
Script:
# Description: Check for references to style.css in the http package, including .vue files # Search for style.css references in .js and .vue files echo "Searching for style.css references in .js and .vue files:" rg "style\.css" packages/http --type-add 'vue:*.vue' # Check if style.css file exists echo "Checking if style.css file exists:" fd -t f "style.css" packages/httpLength of output: 292
designer-demo/registry.js (1)
46-46: LGTM: Addition of GenerateCodeService importThe addition of
GenerateCodeServiceto the imports from '@opentiny/tiny-engine' is consistent with the existing import pattern and aligns with the PR's objective of refactoring the HTTP service.packages/design-core/index.js (1)
Line range hint
1-46: Overall, the changes successfully implement the PR objectives.The addition of the
httpmodule export effectively refactors the HTTP service as a configurable component, aligning with the PR's main objective. The change is minimal yet impactful, maintaining the file's existing structure and export pattern. This modification enhances the modularity of the codebase, allowing for on-demand import of HTTP-related functionalities.A few points to consider:
- Ensure that the
@opentiny/tiny-engine-httpmodule is properly documented, especially regarding its configuration options.- Update any relevant documentation to reflect this new configurable HTTP service.
- Consider adding tests specific to this new export to ensure it behaves as expected when imported and used.
packages/http/src/index.js (4)
28-36: LGTM:preResponsefunction handles response correctlyThe
preResponsefunction properly checks for errors in the response data and returns the expecteddataproperty when no errors are present. This implementation is clear and concise.
38-46: LGTM:errorResponsefunction provides appropriate error handlingThe
errorResponsefunction effectively captures errors, displays an error message to the user usingshowError, and appropriately rejects the promise with the correct error information.
74-80: Flexibility in axios configurationThe
initfunction allowsconfigto be either an object or a function that returns an object. This provides flexibility in configuring axios instances based on dynamic conditions.
90-90: LGTM: Attaching request interceptorsThe request interceptors are correctly attached using
http.interceptors.request.use(...interceptors.request). This ensures any custom request interceptors provided in the options are properly utilized.designer-demo/src/http/index.js (3)
116-126: Ensure Correct Order of Response InterceptorsIn the
httpConfig, the response interceptors are specified as[interceptors.response[0], errorResponse]. Verify that this order is intentional and thaterrorResponsecorrectly handles errors after any prior interceptors.No script needed, but please confirm that
interceptors.response[0]should precedeerrorResponseand that they are compatible.
104-114: Verify Modification ofconfig.baseURLinpreRequestThe
preRequestfunction conditionally setsconfig.baseURLto an empty string. Ensure this does not unintentionally affect API endpoints, especially in production environments.Run the following script to list all API calls and check their base URLs:
#!/bin/bash # Description: List all configurations of baseURL in API requests. rg --type javascript 'config\.baseURL' -A 2
62-72: Verify Behavior When Session ExpiresIn the
errorResponsefunction, when the response status isLOGIN_EXPIRED_CODE, the logic depends on the environment. Verify that in all environments, the user is appropriately prompted to re-authenticate.Run the following script to ensure that the session rebuild process works correctly across different environments:
✅ Verification successful
To address the issues encountered, please run the following scripts to properly verify how session expiration is handled across different environments:
To resolve the regex errors and accurately verify how session expiration is handled across different environments, please run the following scripts with corrected regex patterns:
Session Expiration Handling Verified Successfully
The
errorResponsefunction correctly prompts users to re-authenticate in all environments:
- VSCode Plugin Environment: Detects
window.vscodeBridgeand resolves the session prompt.- Browser Environment: Detects the
x-login-urlheader and invokesopenLogin()to prompt re-authentication.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how session expiration is handled in different environments. # Test for vscodeBridge existence rg --type javascript 'window\.vscodeBridge' -A 3 # Look for handling of 'x-login-url' header rg --type javascript "response\?\.\headers\['x-login-url'\]" -A 3Length of output: 435
Script:
#!/bin/bash # Description: Check how session expiration is handled in different environments using specific file extensions and corrected regex patterns. # Search for vscodeBridge usage in JavaScript files rg 'window\.vscodeBridge' -A 3 --glob '*.js' --glob '*.jsx' # Search for handling of 'x-login-url' header in JavaScript files rg "response\?\.\headers\['x-login-url'\]" -A 3 --glob '*.js' --glob '*.jsx'Length of output: 1671
Script:
#!/bin/bash # Description: Check how session expiration is handled in different environments with corrected regex patterns. # Search for vscodeBridge usage in JavaScript files rg 'window\.vscodeBridge' -A 3 --glob '*.js' --glob '*.jsx' # Search for handling of 'x-login-url' header in JavaScript files with corrected regex rg 'response\?.headers\['"'"'x-login-url'"'"'\]' -A 3 --glob '*.js' --glob '*.jsx'Length of output: 1760
| export { default as http } from '@opentiny/tiny-engine-http' | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Missing @opentiny/tiny-engine-http in package.json Dependencies.
The @opentiny/tiny-engine-http module is being exported and imported across multiple files but is not listed in the package.json dependencies. This can lead to installation and runtime issues.
- Add
@opentiny/tiny-engine-httptodependenciesinpackage.json.
🔗 Analysis chain
New HTTP module export aligns with PR objectives.
The addition of the http module export from @opentiny/tiny-engine-http is consistent with the PR's objective to refactor HTTP as a configurable service. This change allows users to import HTTP-related functionalities on demand, which is in line with the modular structure of the file.
To ensure this change doesn't introduce any conflicts or issues, let's verify the module's existence and its usage across the codebase:
This script will help us confirm that the module is properly included in the project dependencies and identify any existing usage or potential conflicts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of the @opentiny/tiny-engine-http module
echo "Checking for @opentiny/tiny-engine-http in package.json:"
jq '.dependencies["@opentiny/tiny-engine-http"] // .devDependencies["@opentiny/tiny-engine-http"]' package.json
# Check for any existing imports of the http module
echo "Checking for existing imports of the http module:"
rg --type js --type ts 'import.*from.*@opentiny/tiny-engine-http'
# Look for any potential conflicts or duplicate exports
echo "Checking for potential conflicts or duplicate exports:"
rg --type js --type ts 'export.*http.*from'
Length of output: 3414
| http.interceptors.request.use(...interceptors.response) | ||
| } |
There was a problem hiding this comment.
Incorrectly attaching response interceptors to request interceptors
At line 94, response interceptors are being added to the request interceptors chain using http.interceptors.request.use(...interceptors.response). This will result in the response interceptors not being called appropriately and could cause unexpected behaviors in request handling.
Apply this diff to fix the issue:
- http.interceptors.request.use(...interceptors.response)
+ http.interceptors.response.use(...interceptors.response)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| http.interceptors.request.use(...interceptors.response) | |
| } | |
| http.interceptors.response.use(...interceptors.response) | |
| } |
| globalNotify({ | ||
| type: 'error', | ||
| title: '接口报错', | ||
| message: `报错接口: ${url} \n报错信息: ${message ?? ''}` |
There was a problem hiding this comment.
Ensure Error Messages Are Properly Sanitized
When displaying error messages that include dynamic data such as url and message, ensure that they are properly sanitized to prevent potential Cross-Site Scripting (XSS) attacks.
Consider sanitizing the inputs or using a library that handles this automatically.
| if (!procession.promiseLogin) { | ||
| procession.promiseLogin = loginVM.openLogin(procession, '/api/rebuildSession') | ||
| procession.promiseLogin.then(() => { | ||
| getMetaApi('engine.service.http').request(response.config).then(resolve, reject) | ||
| // http.request(response.config).then(resolve, reject) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Avoid Potential Race Conditions in openLogin Function
The openLogin function checks if procession.promiseLogin is falsy before initializing it. However, there might be a race condition if multiple calls happen simultaneously.
Consider using a locking mechanism or initializing procession.promiseLogin atomically to prevent multiple logins from being initiated.
| const { response } = error | ||
|
|
There was a problem hiding this comment.
Handle Undefined response Gracefully
In the errorResponse function, you destructure response from error, but response might be undefined. This could lead to runtime errors when accessing response.status or response.headers.
Apply this diff to add a check for response:
export const errorResponse = (error) => {
// 用户信息失效时，弹窗提示登录
- const { response } = error
+ const { response } = error || {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { response } = error | |
| export const errorResponse = (error) => { | |
| // 用户信息失效时，弹窗提示登录 | |
| const { response } = error || {} |
| const dev = env.MODE?.includes('dev') | ||
|
|
||
| // 获取租户 id | ||
| const getTenant = () => new URLSearchParams(location.search).get('tenant') |
There was a problem hiding this comment.
Ensure location Object Is Available
The getTenant function uses location.search, which may not be available in non-browser environments like Node.js or server-side rendering.
Consider adding a check to ensure location is defined:
const getTenant = () => {
+ if (typeof location === 'undefined') return null
return new URLSearchParams(location.search).get('tenant')
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getTenant = () => new URLSearchParams(location.search).get('tenant') | |
| const getTenant = () => { | |
| if (typeof location === 'undefined') return null | |
| return new URLSearchParams(location.search).get('tenant') | |
| } |
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Chores