[Feature] 通过 WebUI 编辑配置文件 #513
Conversation
WalkthroughThe changes introduce enhancements to configuration management within the application, focusing on the ability to reload configurations dynamically. A new public method, 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 (
|
Deploying pbh-fe with
|
| Latest commit: |
54835f2
|
| Status: | 🚫 Build failed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range and nitpick comments (9)
webui/src/components/codeEditor.vue (1)
28-31: Consider removing the commented-out code.The event handlers for change and input events have their implementations commented out. If this code is not needed, consider removing it to keep the codebase clean.
Apply this diff to remove the commented-out code:
-const onChange = (val: string, cm: Editor) => { - // console.log(val); - // console.log(cm.getValue()); -}; +const onChange = (val: string, cm: Editor) => {}; -const onInput = (val: string) => { - // console.log(val); -}; +const onInput = (val: string) => {};Also applies to: 33-35
webui/package.json (1)
19-19: Consider using a package registry.The addition of the
@codemirror/lang-yamldependency from the GitHub repository is appropriate for providing YAML language support in CodeMirror, which aligns with the PR objective of editing configuration files through a WebUI.However, consider using a package registry like npm instead of directly sourcing from the GitHub repository for better stability and versioning. You can specify a version constraint similar to the other dependencies.
src/main/java/com/ghostchu/peerbanhelper/Main.java (1)
395-400: LGTM!The new
reloadConfigmethod introduces a clean and well-structured functionality for configuration management. The method follows the Single Responsibility Principle and uses thereloadManagerto reload the configurations, which is a good practice.Consider documenting the return type and the expected behavior of the method using Javadoc comments. This will help other developers understand the purpose and usage of the method.
src/main/java/com/ghostchu/peerbanhelper/module/impl/webapi/WebConfigController.java (2)
53-53: Adjust permission level for retrieving configuration contentThe
retrieveConfigContentendpoint currently requiresRole.USER_WRITE. Since it's aGETrequest and only reads data, consider changing the required role toRole.USER_READ.Apply this diff to adjust the role:
.get("/api/config/{configId}", this::retrieveConfigContent, Role.USER_WRITE) + .get("/api/config/{configId}", this::retrieveConfigContent, Role.USER_READ)
26-142: Add unit tests for the new API endpointsIncluding unit tests for the newly added endpoints will help ensure they function as expected and make future refactoring safer.
Consider adding tests to cover:
- Successful retrieval and modification of configurations.
- Handling of invalid
configIdvalues.- Error scenarios like invalid YAML content or I/O failures.
webui/src/views/config/index.vue (4)
27-28: Remove unnecessary custom type definition for the translation functionDefining a custom type
TFunctionfortis unnecessary sincevue-i18nprovides proper typings. This can simplify your code and reduce redundancy.Apply this diff to eliminate the custom type:
- import { useI18n } from 'vue-i18n' // 无需导入 UseI18n - type TFunction = (key: string, values?: Record<string, unknown>) => string + import { useI18n } from 'vue-i18n'
120-121: Adjust the returned properties fromsetup()Since you're using
$tin the template, there's no need to returntfrom thesetup()function. Removing it can clean up your code.Apply this diff to modify the return statement:
return { - t, configYamlList, yamlContents, activeTab, cmOptions, loadTabContent, saveConfig, // Other properties... }
36-38: Specify types usingReffor better type safetyWhen using
refwith TypeScript, it's clearer and safer to specify the types usingRef<T>fromvue. This improves code readability and type checking.Apply this diff to adjust type annotations:
- const configYamlList = ref<string[]>([]) // Holds list of YAML config files - const yamlContents = ref<Record<string, string>>({}) // Holds content of each YAML file - const activeTab = ref<string>('') // Current active tab + const configYamlList = ref<Array<string>>([]) // Holds list of YAML config files + const yamlContents = ref<Record<string, string>>({}) // Holds content of each YAML file + const activeTab = ref('') as Ref<string> // Current active tabEnsure you import
Reffromvueif you use it.
129-130: Remove unnecessary console logs in production codeThe
console.logstatements inonReadyandonFocusmight clutter the console and are generally used for debugging purposes. Consider removing them before deploying.Apply this diff to remove console logs:
onReady: (configId: string) => (cm: Editor) => { - console.log(`Codemirror instance ready for ${configId}`) }, onFocus(cm: Editor, event: FocusEvent) { - console.log('Codemirror focused', cm, event) }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
webui/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (16)
- src/main/java/com/ghostchu/peerbanhelper/Main.java (5 hunks)
- src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java (1 hunks)
- src/main/java/com/ghostchu/peerbanhelper/btn/BtnConfig.java (3 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/IPBlackRuleList.java (2 hunks)
- src/main/java/com/ghostchu/peerbanhelper/module/impl/webapi/WebConfigController.java (1 hunks)
- webui/package.json (2 hunks)
- webui/src/api/model/config.ts (1 hunks)
- webui/src/components/codeEditor.vue (1 hunks)
- webui/src/locale/en-US.ts (3 hunks)
- webui/src/locale/zh-CN.ts (4 hunks)
- webui/src/main.ts (2 hunks)
- webui/src/router/index.ts (1 hunks)
- webui/src/service/config.ts (1 hunks)
- webui/src/views/config/index.vue (1 hunks)
- webui/src/views/config/locale/en-US.ts (1 hunks)
- webui/src/views/config/locale/zh-CN.ts (1 hunks)
Additional comments not posted (31)
webui/src/views/config/locale/zh-CN.ts (1)
1-6: LGTM!The localized messages for the configuration page in Chinese are clear, concise, and cover the essential scenarios. The use of placeholders for dynamic values is a good practice. This file will improve the user experience for Chinese-speaking users.
webui/src/views/config/locale/en-US.ts (1)
1-7: LGTM!The localization file follows best practices and provides comprehensive coverage of user-facing messages for the configuration page. The messages are clear, informative, and use placeholders for dynamic values where appropriate.
The addition of this file enables effective localization support for the WebUI configuration feature.
webui/src/api/model/config.ts (2)
1-18: LGTM!The
ConfigSaveResultinterface provides a clear and well-documented structure for the configuration save result. The properties are aptly named and their purposes are clearly explained in the comments.
20-29: LGTM!The
Resultenum provides a clear set of possible outcomes for the configuration save operation. The enum values are well-defined and cover the necessary scenarios.webui/src/main.ts (2)
11-11: LGTM!The import statement for the
InstallCodeMirrorfunction from thecodemirror-editor-vue3package is syntactically correct.
26-26: CodeMirror editor integration looks good!The
InstallCodeMirrorfunction is correctly used to register the CodeMirror editor as a plugin in the Vue app. This integration will enable powerful code editing features within the application.The placement of the plugin registration after other essential plugins like Pinia, i18n, and the router ensures that the CodeMirror editor can seamlessly interact with different parts of the application.
webui/src/components/codeEditor.vue (2)
1-14: LGTM!The template section is properly structured and uses the correct syntax for rendering the Codemirror editor. The bindings and event handlers are correctly set up.
15-57: The script section is properly structured and set up.The script section is properly structured and uses the correct syntax for setting up the component logic. The reactive references, editor configuration, and lifecycle hooks are correctly set up.
webui/src/service/config.ts (3)
7-18: LGTM!The function is implemented correctly and follows best practices for async/await and response handling.
20-31: LGTM!The function is implemented correctly and follows the same best practices as
getConfigYamlListfor async/await and response handling. TheconfigIdparameter is correctly included in the API endpoint URL.
33-49: LGTM!The function is implemented correctly and follows the same best practices as the previous functions for async/await and response handling. The
configIdparameter is correctly included in the API endpoint URL, and thecontentparameter is correctly sent in the PUT request body.src/main/java/com/ghostchu/peerbanhelper/btn/BtnConfig.java (2)
5-7: LGTM!The imports are necessary for implementing the
Reloadableinterface and using its related classes. The additions are correct and relevant.
19-19: Excellent implementation of theReloadableinterface!The
BtnConfigclass now correctly implements theReloadableinterface, allowing it to support dynamic reloading. ThereloadModulemethod is implemented as required by the interface, and it appropriately returns aReloadResultindicating that a restart is necessary for reloading this module.Requiring a restart for reloading the
BtnConfigmodule seems justified, as it likely contains critical configuration settings that may not be safe to reload dynamically without a restart.The changes are well-structured and adhere to the interface contract. Great work!
Also applies to: 44-47
webui/src/locale/zh-CN.ts (3)
13-13: LGTM!The import statement for
configLocaleis necessary to support localization of configuration-related features.
33-34: Looks good!The new entries for
router.settingsandrouter.settings.configare correctly added to the exported object. The Chinese translations are appropriate for the given keys.
61-61: LGTM!The spread operator is correctly used to include the
configLocalestrings in the final exported object.webui/package.json (4)
18-18: LGTM!The addition of the
@codemirror/lang-jsondependency with version^6.0.1is appropriate for providing JSON language support in CodeMirror, which aligns with the PR objective of editing configuration files through a WebUI.
24-24: LGTM!The addition of the
codemirrordependency with version^5is appropriate for integrating the core CodeMirror functionality, which aligns with the PR objective of editing configuration files through a WebUI.
25-25: LGTM!The addition of the
codemirror-editor-vue3dependency with version^2.8.0is appropriate for integrating CodeMirror with Vue 3, which aligns with the PR objective of editing configuration files through a WebUI.
48-48: LGTM!The addition of the
@types/codemirrordev dependency with version^5.60.15is appropriate for providing TypeScript type definitions for CodeMirror, which ensures type safety during development and aligns with the PR objective of editing configuration files through a WebUI.webui/src/locale/en-US.ts (3)
13-13: LGTM!The import statement for
configLocalefollows the correct syntax and is consistent with the other locale imports in the file.
34-35: LGTM!The new entries for
'router.settings'and'router.settings.config'follow the correct syntax and are consistent with the other entries in the exported default object.
58-59: LGTM!The
configLocaleandchartsLocalemodules are being correctly merged into the exported default object using the spread operator, which is consistent with how the other locale modules are being merged.webui/src/router/index.ts (1)
169-187: LGTM!The new
/settingsroute and its child route/configare properly structured and integrated into the existing router configuration. The routes have appropriate access control by requiring login, and the labels are defined for display purposes. The/configroute correctly loads the configuration view component.Overall, the addition of these routes expands the application's functionality by providing a dedicated section for configuration management.
src/main/java/com/ghostchu/peerbanhelper/Main.java (4)
16-17: LGTM!The new imports are necessary for the
reloadConfigmethod and look good.
107-110: LGTM!The formatting changes improve the readability of the conditional statements.
169-173: LGTM!The formatting changes improve the readability of the conditional statements related to the operating system name.
346-349: LGTM!The formatting changes improve the readability of the conditional statement.
src/main/java/com/ghostchu/peerbanhelper/module/impl/rule/IPBlackRuleList.java (2)
26-26: LGTM!The import statement for the
ReloadStatusclass is valid and does not introduce any issues.
112-113: Excellent asynchronous reload implementation!The changes to the
reloadModulemethod look great:
- Starting a virtual thread to execute the
reloadConfigmethod asynchronously is a good approach to avoid blocking the main thread during the reload operation.- Returning a
ReloadResultwith aSCHEDULEDstatus accurately communicates that the reload has been initiated but not yet completed.The asynchronous nature of the reload aligns well with the overall design and improves responsiveness.
src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java (1)
657-657: LGTM!The registration of the
WebConfigControllermodule with themoduleManageris a valid addition. It aligns with the PR objective of introducing web-based configuration management capabilities.
| javalinWebContainer.javalin() | ||
| .get("/api/config", this::listConfig, Role.USER_READ) | ||
| .get("/api/config/{configId}", this::retrieveConfigContent, Role.USER_WRITE) | ||
| .put("/api/config/{configId}", this::editConfig, Role.USER_WRITE); |
There was a problem hiding this comment.
Potential security risk: Exposing configuration files via API
Allowing users to retrieve and edit configuration files may expose sensitive information if the configurations contain secrets or credentials. Consider restricting access to these endpoints to administrative roles or sanitizing sensitive data before exposure.
Recommendations:
- Restrict Access: Limit these endpoints to admin roles.
- Sanitize Output: Exclude or mask sensitive information in the configuration data.
- Secure Transmission: Ensure that the API uses HTTPS to protect data in transit.
- Audit Logging: Implement logging for access and modification attempts for auditing purposes.
Implement logging for configuration access and modifications
To enhance security and traceability, add logging to record when configurations are listed, retrieved, or modified. This aids in monitoring usage and detecting unauthorized activities.
For example:
private static final Logger logger = LoggerFactory.getLogger(WebConfigController.class);
private void editConfig(Context ctx) {
String user = ctx.sessionAttribute("userId");
String configId = ctx.pathParam("configId");
logger.info("User '{}' is modifying configuration '{}'", user, configId);
// existing code...
}
private void retrieveConfigContent(Context ctx) {
String user = ctx.sessionAttribute("userId");
String configId = ctx.pathParam("configId");
logger.info("User '{}' is retrieving configuration '{}'", user, configId);
// existing code...
}
private void listConfig(Context ctx) {
String user = ctx.sessionAttribute("userId");
logger.info("User '{}' is listing configurations", user);
// existing code...
}Ensure sensitive information is not included in the logs.
Also applies to: 57-79, 81-93, 96-98
| private void editConfig(Context ctx) throws IOException { | ||
| ConfigSaveRequest configSaveRequest = ctx.bodyAsClass(ConfigSaveRequest.class); | ||
| var test = new YamlConfiguration(); | ||
| try { | ||
| test.loadFromString(configSaveRequest.content()); | ||
| } catch (InvalidConfigurationException e) { | ||
| ctx.json(new StdResp(false, "Invalid configuration: " + e.getMessage(), null)); | ||
| return; | ||
| } | ||
| switch (ctx.pathParam("configId")) { | ||
| case "config.yml" -> { | ||
| Files.writeString(Main.getMainConfigFile().toPath(), configSaveRequest.content(), StandardCharsets.UTF_8); | ||
| ctx.json(new StdResp(true, "Configuration saved", processConfigReloading())); | ||
| } | ||
| case "profile.yml" -> { | ||
| Files.writeString(Main.getProfileConfigFile().toPath(), configSaveRequest.content(), StandardCharsets.UTF_8); | ||
| ctx.json(new StdResp(true, "Configuration saved", processConfigReloading())); | ||
| } | ||
| default -> { | ||
| throw new IllegalArgumentException("Invalid configId"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactor to reduce code duplication in handling configuration files
The logic for handling configId in both editConfig and retrieveConfigContent methods is similar. Refactoring this logic into a helper method will improve maintainability.
Implement a helper method to get the configuration file path:
private Path getConfigFilePath(String configId) {
return switch (configId) {
case "config.yml" -> Main.getMainConfigFile().toPath();
case "profile.yml" -> Main.getProfileConfigFile().toPath();
default -> null;
};
}Refactor the methods:
private void editConfig(Context ctx) {
ConfigSaveRequest configSaveRequest = ctx.bodyAsClass(ConfigSaveRequest.class);
var test = new YamlConfiguration();
try {
test.loadFromString(configSaveRequest.content());
} catch (InvalidConfigurationException e) {
ctx.json(new StdResp(false, "Invalid configuration: " + e.getMessage(), null));
return;
}
Path configFilePath = getConfigFilePath(ctx.pathParam("configId"));
if (configFilePath == null) {
ctx.status(400).json(new StdResp(false, "Invalid configId", null));
return;
}
try {
Files.writeString(configFilePath, configSaveRequest.content(), StandardCharsets.UTF_8);
ctx.json(new StdResp(true, "Configuration saved", processConfigReloading()));
} catch (IOException e) {
ctx.json(new StdResp(false, "Error saving configuration: " + e.getMessage(), null));
}
}
private void retrieveConfigContent(Context ctx) {
Path configFilePath = getConfigFilePath(ctx.pathParam("configId"));
if (configFilePath == null) {
ctx.status(400).json(new StdResp(false, "Invalid configId", null));
return;
}
try {
String content = Files.readString(configFilePath, StandardCharsets.UTF_8);
ctx.json(new StdResp(true, null, content));
} catch (IOException e) {
ctx.json(new StdResp(false, "Error reading configuration: " + e.getMessage(), null));
}
}Also applies to: 81-93
Handle exceptions and return appropriate error responses
In the editConfig method, exceptions like IOException during file writing are not caught, which may result in unhandled exceptions and internal server errors. It's important to handle these exceptions to provide meaningful feedback to the client.
Apply this diff to handle IOException and return proper error responses:
private void editConfig(Context ctx) {
ConfigSaveRequest configSaveRequest = ctx.bodyAsClass(ConfigSaveRequest.class);
var test = new YamlConfiguration();
try {
test.loadFromString(configSaveRequest.content());
} catch (InvalidConfigurationException e) {
ctx.json(new StdResp(false, "Invalid configuration: " + e.getMessage(), null));
return;
}
+ try {
switch (ctx.pathParam("configId")) {
case "config.yml" -> {
Files.writeString(Main.getMainConfigFile().toPath(), configSaveRequest.content(), StandardCharsets.UTF_8);
ctx.json(new StdResp(true, "Configuration saved", processConfigReloading()));
}
case "profile.yml" -> {
Files.writeString(Main.getProfileConfigFile().toPath(), configSaveRequest.content(), StandardCharsets.UTF_8);
ctx.json(new StdResp(true, "Configuration saved", processConfigReloading()));
}
default -> {
- throw new IllegalArgumentException("Invalid configId");
+ ctx.status(400).json(new StdResp(false, "Invalid configId", null));
return;
}
}
+ } catch (IOException e) {
+ ctx.json(new StdResp(false, "Error saving configuration: " + e.getMessage(), null));
+ }
}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.
| private void editConfig(Context ctx) throws IOException { | |
| ConfigSaveRequest configSaveRequest = ctx.bodyAsClass(ConfigSaveRequest.class); | |
| var test = new YamlConfiguration(); | |
| try { | |
| test.loadFromString(configSaveRequest.content()); | |
| } catch (InvalidConfigurationException e) { | |
| ctx.json(new StdResp(false, "Invalid configuration: " + e.getMessage(), null)); | |
| return; | |
| } | |
| switch (ctx.pathParam("configId")) { | |
| case "config.yml" -> { | |
| Files.writeString(Main.getMainConfigFile().toPath(), configSaveRequest.content(), StandardCharsets.UTF_8); | |
| ctx.json(new StdResp(true, "Configuration saved", processConfigReloading())); | |
| } | |
| case "profile.yml" -> { | |
| Files.writeString(Main.getProfileConfigFile().toPath(), configSaveRequest.content(), StandardCharsets.UTF_8); | |
| ctx.json(new StdResp(true, "Configuration saved", processConfigReloading())); | |
| } | |
| default -> { | |
| throw new IllegalArgumentException("Invalid configId"); | |
| } | |
| } | |
| } | |
| private void editConfig(Context ctx) { | |
| ConfigSaveRequest configSaveRequest = ctx.bodyAsClass(ConfigSaveRequest.class); | |
| var test = new YamlConfiguration(); | |
| try { | |
| test.loadFromString(configSaveRequest.content()); | |
| } catch (InvalidConfigurationException e) { | |
| ctx.json(new StdResp(false, "Invalid configuration: " + e.getMessage(), null)); | |
| return; | |
| } | |
| try { | |
| switch (ctx.pathParam("configId")) { | |
| case "config.yml" -> { | |
| Files.writeString(Main.getMainConfigFile().toPath(), configSaveRequest.content(), StandardCharsets.UTF_8); | |
| ctx.json(new StdResp(true, "Configuration saved", processConfigReloading())); | |
| } | |
| case "profile.yml" -> { | |
| Files.writeString(Main.getProfileConfigFile().toPath(), configSaveRequest.content(), StandardCharsets.UTF_8); | |
| ctx.json(new StdResp(true, "Configuration saved", processConfigReloading())); | |
| } | |
| default -> { | |
| ctx.status(400).json(new StdResp(false, "Invalid configId", null)); | |
| return; | |
| } | |
| } | |
| } catch (IOException e) { | |
| ctx.json(new StdResp(false, "Error saving configuration: " + e.getMessage(), null)); | |
| } | |
| } |
src/main/java/com/ghostchu/peerbanhelper/module/impl/webapi/WebConfigController.java
Show resolved
Hide resolved
| @change="onChange(configId)" | ||
| @ready="onReady(configId)" | ||
| @focus="onFocus" |
There was a problem hiding this comment.
Ensure correct event handling in Codemirror component
The onChange, onReady, and onFocus event handlers might not be correctly bound due to the way they are defined. Ensure that they are properly set up to avoid unexpected behaviors.
Verify that the event handlers are correctly implemented and consider using .bind or arrow functions if necessary.
webui/src/views/config/index.vue
Outdated
| const loadTabContent = async (key: string | number) => { | ||
| const configId = key.toString(); // 确保 configId 是 string 类型 | ||
| if (!yamlContents.value[configId]) { | ||
| const response = await getConfigYamlContent(configId); | ||
| if (response.success) { | ||
| yamlContents.value[configId] = response.data; | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Handle cases where no configuration files are available
If configYamlList is empty, the component might not behave as expected. Consider adding handling for cases where no configuration files are returned.
Apply this change to handle empty lists:
if (response.success) {
configYamlList.value = response.data
if (configYamlList.value.length > 0) {
activeTab.value = configYamlList.value[0]
await loadTabContent(activeTab.value) // Load content of the first tab
+ } else {
+ Message.warning({ content: 'No configuration files available.', resetOnHover: true })
}
}Committable suggestion was skipped due to low confidence.
| } | ||
|
|
||
| const results = response.data | ||
| let requiresRestart = false | ||
|
|
||
| results.forEach((result) => { | ||
| if (result.result === Result.Exception) { | ||
| Message.error({ | ||
| content: t('page.config.saveFailed', { | ||
| name: result.name, | ||
| errorMsg: result.errorMsg | ||
| }), | ||
| resetOnHover: true | ||
| }) | ||
| return true | ||
| } | ||
| if (result.result === Result.RequireRestart) requiresRestart = true | ||
| }) | ||
|
|
||
| const allSuccessful = results.every( | ||
| (res) => res.result === Result.Success || res.result === Result.Outdated | ||
| ) | ||
|
|
||
| if (allSuccessful) { | ||
| Message.success({ | ||
| content: t('page.config.saveSuccess'), | ||
| resetOnHover: true | ||
| }) | ||
| } | ||
|
|
||
| if (requiresRestart) { | ||
| Message.warning({ | ||
| content: t('page.config.saveRequireRestart'), | ||
| resetOnHover: true | ||
| }) | ||
| } | ||
|
|
||
| return true | ||
| } catch (error: unknown) { | ||
| if (error instanceof Error) { | ||
| Message.error({ content: error.message, resetOnHover: true }) | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
Improve error handling in saveConfig function
The saveConfig function currently returns true regardless of success or failure, which can make it difficult to handle errors effectively. Consider returning false or throwing an error when the save operation fails to provide clearer feedback.
Apply this diff to adjust the return values:
if (!response.success) {
Message.error({ content: response.message, resetOnHover: true })
- return true
+ return false
}
// ...
if (error instanceof Error) {
Message.error({ content: error.message, resetOnHover: true })
}
- return true
+ return falseEnsure that any functions calling saveConfig handle the returned value appropriately.
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 results = response.data | |
| let requiresRestart = false | |
| results.forEach((result) => { | |
| if (result.result === Result.Exception) { | |
| Message.error({ | |
| content: t('page.config.saveFailed', { | |
| name: result.name, | |
| errorMsg: result.errorMsg | |
| }), | |
| resetOnHover: true | |
| }) | |
| return true | |
| } | |
| if (result.result === Result.RequireRestart) requiresRestart = true | |
| }) | |
| const allSuccessful = results.every( | |
| (res) => res.result === Result.Success || res.result === Result.Outdated | |
| ) | |
| if (allSuccessful) { | |
| Message.success({ | |
| content: t('page.config.saveSuccess'), | |
| resetOnHover: true | |
| }) | |
| } | |
| if (requiresRestart) { | |
| Message.warning({ | |
| content: t('page.config.saveRequireRestart'), | |
| resetOnHover: true | |
| }) | |
| } | |
| return true | |
| } catch (error: unknown) { | |
| if (error instanceof Error) { | |
| Message.error({ content: error.message, resetOnHover: true }) | |
| } | |
| return true | |
| } | |
| } | |
| const results = response.data | |
| let requiresRestart = false | |
| results.forEach((result) => { | |
| if (result.result === Result.Exception) { | |
| Message.error({ | |
| content: t('page.config.saveFailed', { | |
| name: result.name, | |
| errorMsg: result.errorMsg | |
| }), | |
| resetOnHover: true | |
| }) | |
| return true | |
| } | |
| if (result.result === Result.RequireRestart) requiresRestart = true | |
| }) | |
| const allSuccessful = results.every( | |
| (res) => res.result === Result.Success || res.result === Result.Outdated | |
| ) | |
| if (allSuccessful) { | |
| Message.success({ | |
| content: t('page.config.saveSuccess'), | |
| resetOnHover: true | |
| }) | |
| } | |
| if (requiresRestart) { | |
| Message.warning({ | |
| content: t('page.config.saveRequireRestart'), | |
| resetOnHover: true | |
| }) | |
| } | |
| return true | |
| } catch (error: unknown) { | |
| if (error instanceof Error) { | |
| Message.error({ content: error.message, resetOnHover: true }) | |
| } | |
| return false | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- webui/src/locale/en-US.ts (3 hunks)
- webui/src/locale/zh-CN.ts (3 hunks)
- webui/src/main.ts (2 hunks)
- webui/src/views/config/index.vue (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- webui/src/locale/en-US.ts
- webui/src/locale/zh-CN.ts
- webui/src/main.ts
Additional context used
GitHub Check: Lint
webui/src/views/config/index.vue
[warning] 2-2:
Attribute 'v-model:activeKey' must be hyphenated
| const loadTabContent = async (key: string | number) => { | ||
| const configId = key.toString() // 确保 configId 是 string 类型 | ||
| if (!yamlContents.value[configId]) { | ||
| const response = await getConfigYamlContent(configId) | ||
| if (response.success) { | ||
| yamlContents.value[configId] = response.data | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling when loading YAML content
In the loadTabContent function, if getConfigYamlContent(configId) fails, there is no error handling to inform the user of the failure.
Consider adding error handling to notify the user when the YAML content fails to load.
Apply this diff to handle errors:
if (!yamlContents.value[configId]) {
const response = await getConfigYamlContent(configId)
if (response.success) {
yamlContents.value[configId] = response.data
+ } else {
+ Message.error({ content: response.message || t('page.config.loadContentFailed'), resetOnHover: true })
+ yamlContents.value[configId] = ''
}
}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 loadTabContent = async (key: string | number) => { | |
| const configId = key.toString() // 确保 configId 是 string 类型 | |
| if (!yamlContents.value[configId]) { | |
| const response = await getConfigYamlContent(configId) | |
| if (response.success) { | |
| yamlContents.value[configId] = response.data | |
| } | |
| } | |
| } | |
| const loadTabContent = async (key: string | number) => { | |
| const configId = key.toString() // 确保 configId 是 string 类型 | |
| if (!yamlContents.value[configId]) { | |
| const response = await getConfigYamlContent(configId) | |
| if (response.success) { | |
| yamlContents.value[configId] = response.data | |
| } else { | |
| Message.error({ content: response.message || t('page.config.loadContentFailed'), resetOnHover: true }) | |
| yamlContents.value[configId] = '' | |
| } | |
| } | |
| } |
| onMounted(async () => { | ||
| const response = await getConfigYamlList() | ||
| if (response.success) { | ||
| configYamlList.value = response.data | ||
| if (configYamlList.value.length > 0) { | ||
| activeTab.value = configYamlList.value[0] | ||
| await loadTabContent(activeTab.value) // Load content of the first tab | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
Add error handling when retrieving the configuration list
In the onMounted hook, if getConfigYamlList() fails (i.e., response.success is false), there is no error handling to inform the user of the failure.
Consider adding error handling to notify the user when the configuration list fails to load.
Apply this diff to handle errors:
if (response.success) {
configYamlList.value = response.data
if (configYamlList.value.length > 0) {
activeTab.value = configYamlList.value[0]
await loadTabContent(activeTab.value) // Load content of the first tab
}
+ } else {
+ Message.error({ content: response.message || t('page.config.loadListFailed'), resetOnHover: true })
}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.
| onMounted(async () => { | |
| const response = await getConfigYamlList() | |
| if (response.success) { | |
| configYamlList.value = response.data | |
| if (configYamlList.value.length > 0) { | |
| activeTab.value = configYamlList.value[0] | |
| await loadTabContent(activeTab.value) // Load content of the first tab | |
| } | |
| } | |
| }) | |
| onMounted(async () => { | |
| const response = await getConfigYamlList() | |
| if (response.success) { | |
| configYamlList.value = response.data | |
| if (configYamlList.value.length > 0) { | |
| activeTab.value = configYamlList.value[0] | |
| await loadTabContent(activeTab.value) // Load content of the first tab | |
| } | |
| } else { | |
| Message.error({ content: response.message || t('page.config.loadListFailed'), resetOnHover: true }) | |
| } | |
| }) |
|
webui编辑配置请通过表单的方式完成 |
|
放弃吧，我是不会接受的 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
webui/src/locale/zh-CN.ts (1)
37-38: LGTM: New localization entries for settings and configuration.The additions for 'router.settings' and 'router.settings.config' provide necessary localization for the new WebUI configuration editing feature. The spread of
configLocaleensures all configuration-related localizations are included.For consistency with other spread operators in this file, consider moving
...configLocaleto be grouped with similar spreads.Consider applying this change for better grouping:
...ruleManageMentLocale, - ...chartsLocale, - ...configLocale + ...chartsLocale, + ...configLocaleAlso applies to: 64-64
webui/package.json (2)
19-19: Consider using a specific version for @codemirror/lang-yamlThe addition of YAML language support is beneficial for editing configuration files. However, specifying the package using a GitHub URL may lead to potential instability or unexpected changes in the future.
Consider using a specific version number instead of the GitHub URL if a stable release is available. This would ensure better control over updates and maintain consistency across different environments.
24-24: Consider upgrading to CodeMirror 6 and specifying a more precise versionWhile adding CodeMirror is appropriate for the web-based configuration editing feature, there are two points to consider:
CodeMirror 5 is an older version. Consider upgrading to CodeMirror 6 for the latest features and improvements, unless there are specific compatibility requirements for version 5.
The current version specification "^5" is quite broad and could lead to unexpected behavior if there are breaking changes within the 5.x.x range.
Consider upgrading to CodeMirror 6 if possible. If you need to stick with version 5, it's recommended to specify a more precise version range, such as "^5.65.0" (replace with the minimum version you need).
webui/src/router/index.ts (1)
196-214: LGTM! Consider adding an icon for consistency.The new settings route and its child route for configuration are well-structured and consistent with the existing routing pattern. The use of dynamic import for the component is a good practice for code splitting.
For consistency with other top-level routes, consider adding an icon to the settings route metadata. This would enhance the visual consistency in the navigation UI if icons are used.
You could add an icon like this:
import { IconSettings } from '@arco-design/web-vue/es/icon' // In the route configuration meta: { label: 'router.settings', needLogin: true, icon: () => h(IconSettings) }src/main/java/com/ghostchu/peerbanhelper/Main.java (2)
Line range hint
108-172: LGTM: Improved proxy settings setup.The changes to the
setupProxySettings()method are good improvements:
- Using
varforproxySectionimproves readability.- The early return for null
proxySectionis a good practice.- The updated switch expression syntax is more concise.
Consider extracting the proxy configuration keys (e.g., "host", "port", "non-proxy-hosts") into constants to improve maintainability.
396-401: LGTM: NewreloadConfig()method added.The new
reloadConfig()method is a good addition:
- It provides a way to dynamically reload configurations.
- It ensures proxy settings are updated after reloading.
- It leverages the
ReloadManagerfor handling reloads.Consider adding a log statement at the beginning of the method to indicate that a configuration reload has been initiated. This would be helpful for debugging and monitoring.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
webui/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
- src/main/java/com/ghostchu/peerbanhelper/Main.java (5 hunks)
- src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java (1 hunks)
- webui/package.json (2 hunks)
- webui/src/locale/en-US.ts (3 hunks)
- webui/src/locale/zh-CN.ts (3 hunks)
- webui/src/router/index.ts (1 hunks)
🧰 Additional context used
🪛 Biome
webui/src/locale/en-US.ts
[error] 10-10: Shouldn't redeclare 'chartsLocale'. Consider to delete it or rename it.
'chartsLocale' is defined here:
(lint/suspicious/noRedeclare)
webui/src/locale/zh-CN.ts
[error] 10-10: Shouldn't redeclare 'chartsLocale'. Consider to delete it or rename it.
'chartsLocale' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (13)
webui/src/locale/zh-CN.ts (2)
11-11: LGTM: New import for configuration localization.The import of
configLocaleis correctly placed and follows the established pattern for locale imports. This addition aligns with the PR objective of implementing WebUI configuration editing.
Line range hint
1-64: Summary: Localization updates for WebUI configuration editing feature.The changes to this file appropriately support the new WebUI configuration editing feature by adding necessary localizations. The additions are well-placed and consistent with the existing structure.
Please address the duplicate import issue and consider the suggestion for grouping the spread operators. Once these minor adjustments are made, the localization support for the new feature will be complete and well-integrated.
🧰 Tools
🪛 Biome
[error] 10-10: Shouldn't redeclare 'chartsLocale'. Consider to delete it or rename it.
'chartsLocale' is defined here:
(lint/suspicious/noRedeclare)
webui/package.json (4)
18-18: LGTM: Addition of @codemirror/lang-json packageThe addition of the "@codemirror/lang-json" package is appropriate for implementing JSON editing functionality in the WebUI. This aligns well with the PR objective of allowing users to edit configuration files through a web interface.
25-25: LGTM: Addition of codemirror-editor-vue3 packageThe addition of the "codemirror-editor-vue3" package is appropriate for integrating CodeMirror with Vue 3. This aligns well with the project structure and the goal of implementing a web-based configuration editor.
48-48: LGTM: Addition of @types/codemirror packageThe addition of TypeScript type definitions for CodeMirror is a good practice and will improve development experience with better type checking and autocompletion.
Note that these type definitions are for CodeMirror 5.x. If you decide to upgrade to CodeMirror 6 as suggested in a previous comment, make sure to update these type definitions accordingly.
Line range hint
18-48: Summary: Package additions align well with PR objectivesThe additions to the
package.jsonfile are consistent with the PR objective of implementing a web-based configuration editor. The inclusion of CodeMirror and its related packages (JSON and YAML language support, Vue 3 integration, and TypeScript definitions) provides a solid foundation for creating a rich text editing experience for configuration files.While the additions are approved, consider the following recommendations:
- Evaluate the possibility of upgrading to CodeMirror 6 for the latest features and improvements.
- Use more specific version ranges where possible to ensure better stability and predictability.
- Ensure that the YAML language support package is using a stable, versioned release if available.
These changes demonstrate a thoughtful approach to implementing the new feature while maintaining good development practices.
webui/src/locale/en-US.ts (3)
11-11: LGTM: New import forconfigLocale.The addition of
configLocaleimport is consistent with the PR objectives and follows the existing pattern for importing view-specific locales.
38-39: LGTM: New router entries for settings and config.The addition of 'router.settings' and 'router.settings.config' entries is consistent with the PR objectives and follows the existing naming convention for router locales. The hierarchy (settings > config) is logical and aligns with typical UI structures.
64-65: LGTM: Addition ofconfigLocaleto the export.The inclusion of
configLocalein the spread operator ensures that the new config-related locale strings are properly exported. This follows the existing pattern for other view-specific locales and is consistent with the PR objectives.src/main/java/com/ghostchu/peerbanhelper/Main.java (3)
16-17: LGTM: New imports added correctly.The new imports for
ReloadResultandReloadableContainerare appropriately added and will be used in the newreloadConfig()method.
347-349: LGTM: Improved formatting indecapitalize()method.The formatting changes in the
decapitalize()method improve readability without altering the functionality. The logic remains correct for its intended purpose.
Line range hint
1-401: Overall LGTM: Changes align with PR objectives and improve code quality.The changes in this file successfully implement the functionality required for the WebUI configuration editing feature:
- The new
reloadConfig()method enables dynamic reloading of configurations, which is essential for the WebUI editing feature.- Improvements in the
setupProxySettings()method enhance the overall robustness of the configuration handling.- Code readability and maintainability have been improved through better formatting and use of modern Java features.
These changes align well with the PR objectives of allowing configuration editing through a WebUI interface. The implementation provides a solid foundation for the feature while maintaining good code quality.
src/main/java/com/ghostchu/peerbanhelper/PeerBanHelperServer.java (1)
672-672: LGTM: WebConfigController successfully addedThe addition of
WebConfigControlleris consistent with the PR objective of allowing users to edit configuration files through a WebUI. Its registration is appropriately placed among other web-related controllers.
| @@ -7,6 +7,8 @@ import loginLocale from '@/views/login/locale/zh-CN' | |||
| import oobeLocale from '@/views/oobe/locale/zh-CN' | |||
| import topBanPageLocale from '@/views/ranks/locale/zh-CN' | |||
| import ruleManageMentLocale from '@/views/rule-management/locale/zh-CN' | |||
| import chartsLocale from '@/views/charts/locale/zh-CN' | |||
There was a problem hiding this comment.
Remove duplicate import of chartsLocale.
This line duplicates the import of chartsLocale from line 3. Duplicate imports are unnecessary and can lead to confusion.
Please remove this line:
-import chartsLocale from '@/views/charts/locale/zh-CN'📝 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.
| import chartsLocale from '@/views/charts/locale/zh-CN' |
🧰 Tools
🪛 Biome
[error] 10-10: Shouldn't redeclare 'chartsLocale'. Consider to delete it or rename it.
'chartsLocale' is defined here:
(lint/suspicious/noRedeclare)
| @@ -7,6 +7,8 @@ import loginLocale from '@/views/login/locale/en-US' | |||
| import oobeLocale from '@/views/oobe/locale/en-US' | |||
| import topBanPageLocale from '@/views/ranks/locale/en-US' | |||
| import ruleManageMentLocale from '@/views/rule-management/locale/en-US' | |||
| import chartsLocale from '@/views/charts/locale/en-US' | |||
There was a problem hiding this comment.
Remove duplicate import of chartsLocale.
The chartsLocale is already imported on line 4. This duplicate import on line 10 is unnecessary and should be removed to avoid potential confusion or errors.
Apply this diff to remove the duplicate import:
-import chartsLocale from '@/views/charts/locale/en-US'📝 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.
| import chartsLocale from '@/views/charts/locale/en-US' |
🧰 Tools
🪛 Biome
[error] 10-10: Shouldn't redeclare 'chartsLocale'. Consider to delete it or rename it.
'chartsLocale' is defined here:
(lint/suspicious/noRedeclare)
|
我要提议这个功能作为 GUI 的后备，“直接编辑” |
|
心不甘情不愿，就算是没有菜单，也请把路由留下，我必须挣扎一下 |
本 PR 由 ChatGPT 4o 提供技术支持（前端）。
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Localization
Dependencies