restart when developer sandbox is toggled #1232
Conversation
WalkthroughThis pull request makes several changes to improve deployment commands and settings management. In the API justfile, the deployment command now accepts a remote parameter, and clearer aliases replace the shorthand. The Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
api/src/unraid-api/graph/connect/connect.resolver.ts (1)
70-77: Well-implemented restart logic.The implementation correctly checks if a restart is required after syncing settings and schedules the restart with a delay to allow the current request to complete.
One suggestion: consider extracting the timeout value to a constant for better maintainability.
+ private readonly API_RESTART_DELAY_MS = 3_000; public async updateApiSettings(@Args('input') settings: ApiSettingsInput) { this.logger.verbose(`Attempting to update API settings: ${JSON.stringify(settings, null, 2)}`); const restartRequired = await this.connectSettingsService.syncSettings(settings); const currentSettings = await this.connectSettingsService.getCurrentSettings(); if (restartRequired) { setTimeout(async () => { await this.connectService.restartApi(); - }, 3_000); + }, this.API_RESTART_DELAY_MS); } return currentSettings; }api/src/unraid-api/graph/connect/connect-settings.service.ts (1)
274-274: Indentation issue in description string.The static analysis tool indicates an indentation issue with this line.
- const description = 'The developer sandbox is available at <code><a class="underline" href="/graphql" target="_blank">/graphql</a></code>.'; + const description = 'The developer sandbox is available at <code><a class="underline" href="/graphql" target="_blank">/graphql</a></code>.';Split the string across multiple lines with proper indentation for better readability:
- const description = 'The developer sandbox is available at <code><a class="underline" href="/graphql" target="_blank">/graphql</a></code>.'; + const description = + 'The developer sandbox is available at <code><a class="underline" href="/graphql" target="_blank">/graphql</a></code>.';🧰 Tools
🪛 GitHub Check: Build API
[failure] 274-274:
Insert⏎···········🪛 GitHub Check: Test API
[failure] 274-274:
Insert⏎···········
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
api/justfile(1 hunks)api/src/unraid-api/graph/connect/connect-settings.service.ts(5 hunks)api/src/unraid-api/graph/connect/connect.resolver.ts(2 hunks)api/src/unraid-api/graph/connect/connect.service.ts(1 hunks)unraid-ui/src/forms/Switch.vue(2 hunks)web/components/ConnectSettings/ConnectSettings.ce.vue(6 hunks)
🧰 Additional context used
🪛 GitHub Check: Build API
api/src/unraid-api/graph/connect/connect.resolver.ts
[failure] 20-20:
import statements should have an absolute path
api/src/unraid-api/graph/connect/connect-settings.service.ts
[failure] 274-274:
Insert ⏎···········
🪛 GitHub Check: Test API
api/src/unraid-api/graph/connect/connect.resolver.ts
[failure] 20-20:
import statements should have an absolute path
api/src/unraid-api/graph/connect/connect-settings.service.ts
[failure] 274-274:
Insert ⏎···········
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (20)
unraid-ui/src/forms/Switch.vue (2)
2-3: Good implementation of the description computed property.The computed property correctly extracts the description from the uischema options, providing a clean way to access this data.
Also applies to: 17-17
22-22: Consider the security implications of using v-html.While v-html works well for rendering HTML content in the description, be aware that it bypasses Vue's built-in XSS protections. If the description content comes from user input or an untrusted source, this could pose a security risk.
Consider using a safer alternative if the HTML rendering isn't strictly necessary:
- <p v-if="description" v-html="description" class="mb-2"></p> + <p v-if="description" class="mb-2">{{ description }}</p>Or if HTML is required, ensure the content is sanitized before display.
api/src/unraid-api/graph/connect/connect.service.ts (1)
1-3: Good implementation of the API restart functionality.The implementation correctly handles error logging and uses execa to execute the shell command. The Logger is properly instantiated with the class name for context.
Also applies to: 7-14
api/justfile (2)
12-13: Good enhancement to the deploy command.Adding a remote parameter to the deploy command improves flexibility by allowing deployment to different targets.
15-16: Clear and intuitive aliases.The aliases 'b' and 'd' are intuitive shorthand for build and deploy, making the commands easier to use.
api/src/unraid-api/graph/connect/connect.resolver.ts (1)
25-28: Good dependency injection of ConnectService.The constructor properly injects the ConnectService, following dependency injection best practices.
web/components/ConnectSettings/ConnectSettings.ce.vue (7)
8-8: Import reorganization looks good.The imports have been reorganized in a cleaner way, using destructuring to import multiple components from the
@unraid/uipackage in a single line.
20-20: Good update to destructure the refetch function.Now destructuring both
resultandrefetchfrom theuseQueryhook. This is necessary to support the new functionality that refreshes settings after updates.
30-32: Effective implementation of restart detection.The
restartRequiredcomputed property correctly checks if the sandbox setting has changed, which will determine if an API restart is needed after applying settings.
60-62: Enhanced user feedback with conditional restart notification.The success toast now includes additional context about the API restarting when relevant, which improves the user experience by providing clear feedback about what's happening.
74-74: Simplified renderer declaration.The renderers array declaration is now more concise using the spread operator.
80-80: Important addition of refetch after mutation.Adding a refetch after updating settings ensures the UI displays the latest data from the backend after changes are applied.
119-119: Clear user notification about restart.This informative message clearly communicates to users that the API will restart after settings are applied when necessary, setting appropriate expectations.
api/src/unraid-api/graph/connect/connect-settings.service.ts (7)
64-66: Return type for syncSettings improved with documentation.The method now explicitly returns a boolean to indicate whether a restart is required, and this is properly documented. This enhances the API contract clarity.
Also applies to: 98-99
67-67: Good use of flag variable.The
restartRequiredflag is effectively used to track restart requirements throughout the method.
94-94: Elegant use of compound assignment operator.Using the
||=operator to accumulate restart status is a concise approach that ensures any restart requirement from any setting change is captured.
107-118: Well-documented and improved sandbox mode setter.The method has been enhanced to:
- Clearly document its purpose and return value
- Check if the sandbox mode actually changed before applying updates
- Return a boolean indicating whether a change occurred
This prevents unnecessary restarts when the value hasn't changed.
152-152: Correctly updated to await the result.Updated to properly await the now-async
sandboxSlicemethod.
272-274: Made sandboxSlice method async for better state access.Converting the method to async allows it to fetch the current sandbox state, which is used to determine whether to show the description.
🧰 Tools
🪛 GitHub Check: Build API
[failure] 274-274:
Insert⏎···········🪛 GitHub Check: Test API
[failure] 274-274:
Insert⏎···········
290-290: Good conditional description display.Only showing the description when sandbox is enabled improves user experience by providing relevant information exactly when needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/unraid-api/graph/connect/connect.resolver.ts (1)
69-77: Added API restart logic when settings require itThe updateApiSettings method now checks if a restart is required after syncing settings, and if so, schedules an API restart after a 3-second delay. This allows the response to be sent to the client before restarting.
Consider extracting the 3000ms delay to a named constant for better readability and maintainability:
- }, 3_000); + }, API_RESTART_DELAY_MS);Where
API_RESTART_DELAY_MSwould be defined as a class property or module constant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
api/src/unraid-api/graph/connect/connect.resolver.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
api/src/unraid-api/graph/connect/connect.resolver.ts (1)
api/src/graphql/generated/api/types.ts (2) (2)
Resolver(1862:1862)ConnectResolvers(2297:2302)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
api/src/unraid-api/graph/connect/connect.resolver.ts (2)
19-19: Import added for ConnectService using absolute pathThis correctly follows the project's convention of using absolute paths for imports, addressing a previously flagged issue.
24-27: Properly injected ConnectService dependencyThe constructor has been updated to include the ConnectService as a dependency, following good dependency injection practices.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Summary by CodeRabbit
New Features
Bug Fixes