fix(huicharts): fix the script error of Huicharts saas theme #2887
Conversation
WalkthroughThe pull request updates the theme replacement logic in the build-chart-theme file. It changes the target from Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Command Line Interface
participant BCT as build-chart-theme Function
participant ThemeProc as Theme Processor
CLI->>BCT: Invoke chart theme build command
BCT->>ThemeProc: Process theme value replacement using huiChartOption.theme
ThemeProc-->>BCT: Return updated theme configuration
BCT-->>CLI: Output processed chart theme
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
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 (
|
WalkthroughThis pull request addresses a script error in the Huicharts SaaS theme by modifying the theme assignment logic in the build-chart-theme.ts file. The change involves updating the regular expression pattern used to replace the theme value in the file content. Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
![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 (1)
internals/cli/src/commands/build/build-chart-theme.ts (1)
27-34: 🛠️ Refactor suggestionAdd error handling for file operations.
File operations could fail and should be handled gracefully.
Add try-catch blocks for file operations:
-// 读取文件内容 -let content = fs.readFileSync(filePath, 'utf-8') +let content +try { + // 读取文件内容 + content = fs.readFileSync(filePath, 'utf-8') +} catch (error) { + console.error(`Error reading file: ${error.message}`) + process.exit(1) +} // 替换 process.env.VAR_NAME 为对应主题的值 content = content.replace(/huiChartOption\.theme\s=\s'[a-zA-Z_-]+'/g, `huiChartOption.theme = '${theme}'`) -// 写入新内容到文件 -fs.writeFileSync(filePath, content, 'utf-8') +try { + // 写入新内容到文件 + fs.writeFileSync(filePath, content, 'utf-8') +} catch (error) { + console.error(`Error writing file: ${error.message}`) + process.exit(1) +}
🧹 Nitpick comments (3)
internals/cli/src/commands/build/build-chart-theme.ts (3)
16-22: Enhance error handling and logging.The current error handling is minimal and could be improved for better debugging and user experience.
Consider applying these improvements:
// 获取命令行参数 const theme = THEMES[process.argv[3]] || process.argv[3] || THEMES.bpit // 确保提供了正确的主题参数 if (!theme) { + console.error('Error: Invalid theme specified') process.exit(1) } + +// Log selected theme for debugging +console.log(`Applying theme: ${theme}`)
24-25: Consider making the file path configurable.The hardcoded file path could cause issues in different environments or project structures.
Consider accepting the path as a parameter or environment variable:
-const filePath = path.join(__dirname, '../../../../../packages/vue/src/huicharts/huicharts-core/src/chart-core.ts') +const basePath = process.env.HUICHARTS_PATH || '../../../../../packages/vue/src/huicharts/huicharts-core/src' +const filePath = path.join(__dirname, basePath, 'chart-core.ts')
37-39: Add documentation and return value.The exported function would benefit from documentation and a return value indicating success/failure.
Consider these improvements:
+/** + * Updates the Huicharts theme in the chart-core.ts file. + * @returns {boolean} True if theme was successfully updated, false otherwise. + */ export const chartTheme = () => { - changeTheme() + try { + changeTheme() + return true + } catch (error) { + console.error('Failed to update theme:', error) + return false + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internals/cli/src/commands/build/build-chart-theme.ts(1 hunks)
🔇 Additional comments (2)
internals/cli/src/commands/build/build-chart-theme.ts (2)
5-14: LGTM! Well-structured theme configuration.The theme configuration follows a consistent naming pattern and provides good coverage for different design systems with their light/dark variants.
31-31: Verify the regex pattern change.The regex pattern was changed to target
huiChartOption.theme. Let's verify this change is consistent with the codebase.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for theme assignments in the target file to verify the pattern rg "theme\s*=\s*['\"]" "packages/vue/src/huicharts/huicharts-core/src/chart-core.ts"Length of output: 176
The regex now correctly targets theme assignments as seen in the file:
- Verified usage: In packages/vue/src/huicharts/huicharts-core/src/chart-core.ts, the lines
match the updated regex pattern.huiChartOption.theme = 'cloud-light'There were no instances found (via our checks) of theme assignments using double quotes that the regex might miss, so the change is consistent with the codebase.
PR
修复huicharts图表saas主题脚本错误问题
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
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
These improvements streamline the process of configuring charts, providing users with a more reliable and customizable experience.