chore: improve dx for scripting & setup #969
Conversation
WalkthroughThis pull request includes various updates across multiple files, primarily focusing on configuration adjustments and the introduction of new commands for project management. Key changes include the addition of a newline in the Changes
Possibly related PRs
🪧 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: 3
🧹 Outside diff range and nitpick comments (14)
web/justfile (2)
3-5: Add documentation for list-commands recipeWhile the default command implementation looks good, it would be helpful to add a comment explaining what
list-commandsincludes, as it's not immediately clear what additional commands it provides.default: + # Lists web project recipes followed by additional commands from list-commands @just --list --justfile {{justfile()}} --list-heading $'\nWeb project recipes:\n' @just list-commands
7-9: Enhance setup command robustness and functionalityWhile the basic setup steps are good, consider enhancing the setup command with additional checks and features for a better developer experience.
setup: - cp .env.example .env - npm install + #!/usr/bin/env sh + # Ensure .env.example exists + if [ ! -f .env.example ]; then + echo "Error: .env.example not found" + exit 1 + fi + + # Check if .env already exists + if [ -f .env ]; then + echo "Warning: .env already exists. Skipping copy to prevent overwrite." + else + cp .env.example .env + echo "Created .env from .env.example" + fi + + # Verify node version matches .nvmrc + if [ -f .nvmrc ]; then + expected_version=$(cat .nvmrc) + if ! node -v | grep -q $expected_version; then + echo "Warning: Node version doesn't match .nvmrc" + fi + fi + + # Install dependencies + echo "Installing dependencies..." + if ! npm install; then + echo "Error: npm install failed" + exit 1 + fi + + echo "Setup completed successfully!"api/justfile (1)
7-9: Enhance setup command robustness and documentationConsider these improvements for better developer experience:
- Add prerequisite checks for npm and container runtime
- Add error handling for failed commands
- Document the required environment (npm version, container runtime)
- Consider adding a
--helpflag for setup instructionsHere's a suggested enhancement:
setup: + #!/usr/bin/env sh + # Verify prerequisites + command -v npm >/dev/null 2>&1 || { echo "npm is required but not installed. Aborting." >&2; exit 1; } + command -v docker >/dev/null 2>&1 || { echo "docker is required but not installed. Aborting." >&2; exit 1; } + # Install dependencies npm install + if [ $? -ne 0 ]; then + echo "npm install failed. Please check your npm configuration and try again." >&2 + exit 1 + fi + # Build container npm run container:build + if [ $? -ne 0 ]; then + echo "Container build failed. Please check your Docker configuration and try again." >&2 + exit 1 + fijustfile (3)
6-7: Consider using a more portable string format.While the current implementation works, using
$'\n'might not be portable across all shells. Consider using a more compatible approach.- @just --list --justfile {{justfile()}} --list-heading $'\nMonorepo recipes:\n' + @echo "\nMonorepo recipes:\n" + @just --list --justfile {{justfile()}}
16-17: Consider adding error handling for setup commands.The setup commands for api and web components should handle potential failures gracefully.
- just api/setup - just web/setup + just api/setup || echo "Warning: API setup failed" + just web/setup || echo "Warning: Web setup failed"
34-35: Consider improving the list-ignored command output.The current implementation might be hard to read. Consider adding some formatting and explanation of the output.
-list-ignored: - -git ls-files -v | grep '^S' +list-ignored: + @echo "Files being ignored (marked with 'S'):" + @git ls-files -v | grep '^S' | sed 's/^S //'readme.md (8)
37-37: Update demo link destinationThe "View Demo" link currently points to the repository instead of an actual demo. Consider either:
- Removing the demo link if no demo is available
- Updating it to point to an actual demo instance
102-106: Standardize list formattingFor consistency with the rest of the document, consider using asterisks (*) instead of dashes (-) for unordered lists.
- - Bash - - Docker (for macOS folks, Orbstack works too) - - [Node.js (v20)][Node-url] - - [Just](https://github.com/casey/just) - - An [Unraid][Unraid-url] server for development + * Bash + * Docker (for macOS folks, Orbstack works too) + * [Node.js (v20)][Node-url] + * [Just](https://github.com/casey/just) + * An [Unraid][Unraid-url] server for development🧰 Tools
🪛 Markdownlint (0.35.0)
102-102: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
103-103: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
104-104: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
105-105: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
106-106: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
125-128: Add explanation for thejustcommandThe setup instructions mention using the
justcommand without explaining what it does. Consider adding a brief explanation of what the setup command accomplishes.🧰 Tools
🪛 LanguageTool
[style] ~128-~128: In American English, abbreviations like “etc.” require a period.
Context: ...e (i.e. personal editor customizations, etc). 3. Run the API container ```sh ...(ETC_PERIOD)
128-128: Fix punctuationAdd a period after "etc" for proper American English usage.
- This will run installation scripts, container builds, and some git scripts to reduce noise (i.e. personal editor customizations, etc). + This will run installation scripts, container builds, and some git scripts to reduce noise (i.e. personal editor customizations, etc.).🧰 Tools
🪛 LanguageTool
[style] ~128-~128: In American English, abbreviations like “etc.” require a period.
Context: ...e (i.e. personal editor customizations, etc). 3. Run the API container ```sh ...(ETC_PERIOD)
151-151: Replace placeholder usage contentThe usage section currently contains template placeholder text. Consider adding actual usage examples specific to the Unraid Connect project.
204-204: Fix Twitter handle formattingThe Twitter handle is using a bare URL. Consider using proper markdown link formatting.
- [@UnraidOfficial](https://twitter.com/UnraidOfficial) - support@unraid.net.com + [@UnraidOfficial](https://twitter.com/UnraidOfficial) - [support@unraid.net.com](mailto:support@unraid.net.com)🧰 Tools
🪛 Markdownlint (0.35.0)
204-204: null
Bare URL used(MD034, no-bare-urls)
214-216: Remove empty acknowledgment linksThe acknowledgments section contains empty links that should either be populated with actual content or removed.
- * []() - * []() - * []()🧰 Tools
🪛 Markdownlint (0.35.0)
214-214: null
No empty links(MD042, no-empty-links)
215-215: null
No empty links(MD042, no-empty-links)
216-216: null
No empty links(MD042, no-empty-links)
236-236: Remove unused reference definitionThe
product-screenshotreference is defined but not used in the document. Consider removing it or adding the screenshot where appropriate.🧰 Tools
🪛 Markdownlint (0.35.0)
236-236: Unused link or image reference definition: "product-screenshot"
Link and image reference definitions should be needed(MD053, link-image-reference-definitions)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
.nvmrc(1 hunks).vscode/settings.json(0 hunks)api/.vscode/settings.json(1 hunks)api/justfile(1 hunks)justfile(1 hunks)readme.md(1 hunks)web/.env.example(1 hunks)web/justfile(1 hunks)
💤 Files with no reviewable changes (1)
- .vscode/settings.json
✅ Files skipped from review due to trivial changes (3)
- .nvmrc
- api/.vscode/settings.json
- web/.env.example
🧰 Additional context used
🪛 LanguageTool
readme.md
[style] ~13-~13: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 1881 characters long)
Context: ...contributors-shield]][contributors-url] [![Forks][forks-shield]][forks-url] [![Stargazers][stars-shield]][stars-url] [![Issues][issues-shield]][issues-url] [![MIT License][license-shield]][license-u...
(EN_EXCESSIVE_EXCLAMATION)
[style] ~128-~128: In American English, abbreviations like “etc.” require a period.
Context: ...e (i.e. personal editor customizations, etc). 3. Run the API container ```sh ...
(ETC_PERIOD)
🪛 Markdownlint (0.35.0)
readme.md
102-102: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
103-103: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
104-104: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
105-105: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
106-106: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
204-204: null
Bare URL used
(MD034, no-bare-urls)
214-214: null
No empty links
(MD042, no-empty-links)
215-215: null
No empty links
(MD042, no-empty-links)
216-216: null
No empty links
(MD042, no-empty-links)
236-236: Unused link or image reference definition: "product-screenshot"
Link and image reference definitions should be needed
(MD053, link-image-reference-definitions)
🔇 Additional comments (5)
web/justfile (1)
1-2: LGTM! Good use of fallback setting
The fallback setting enables command inheritance from parent justfiles, which is ideal for monorepo setups.
api/justfile (2)
3-5:
Verify the existence of list-commands recipe
The default command calls list-commands, but this recipe isn't defined in this file. Please ensure it exists in an imported justfile or consider defining it here.
✅ Verification successful
The verification shows that the list-commands recipe exists in the root justfile, which is accessible to the api/justfile through Just's import mechanism. Since the recipe is properly defined and available, I can generate the final response:
The list-commands recipe is properly defined
The list-commands recipe called in the default command is defined in the root justfile and is accessible through Just's import mechanism, making this a valid and working configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for list-commands recipe in other justfiles
rg -A 5 "list-commands:" --type-add 'just:**/justfile'
Length of output: 344
1-1: Consider security implications of fallback behavior
The set fallback directive allows undefined recipe names to be executed as shell commands. This could potentially lead to unintended command execution. Consider explicitly defining all needed commands instead.
justfile (2)
1-4: LGTM! Good default behavior choice.
Setting list-commands as the default recipe provides excellent developer experience by showing available commands when running just without arguments.
14-14: Verify the necessity of hardcoded paths.
The hardcoded paths api/dev/states/myservers.cfg and api/dev/Unraid.net/myservers.cfg seem very specific. Consider making these paths configurable or documenting why they're necessary.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
readme.md (4)
102-106: Standardize markdown list styleFor consistency with markdown best practices, consider using asterisks instead of dashes for unordered lists.
- - Bash - - Docker (for macOS folks, Orbstack works too) - - [Node.js (v20)][Node-url] - - [Just](https://github.com/casey/just) - - An [Unraid][Unraid-url] server for development + * Bash + * Docker (for macOS folks, Orbstack works too) + * [Node.js (v20)][Node-url] + * [Just](https://github.com/casey/just) + * An [Unraid][Unraid-url] server for development🧰 Tools
🪛 Markdownlint (0.35.0)
102-102: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
103-103: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
104-104: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
105-105: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
106-106: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
128-128: Fix minor formatting issues
- Add a period after "etc" in the installation instructions
- Use proper markdown link syntax for the Twitter handle
- This will run installation scripts, container builds, and some git scripts to reduce noise (i.e. personal editor customizations, etc). + This will run installation scripts, container builds, and some git scripts to reduce noise (i.e. personal editor customizations, etc.). - [@UnraidOfficial](https://twitter.com/UnraidOfficial) - support@unraid.net.com + [@UnraidOfficial](https://twitter.com/UnraidOfficial) - [support@unraid.net.com](mailto:support@unraid.net.com)Also applies to: 204-204
🧰 Tools
🪛 LanguageTool
[style] ~128-~128: In American English, abbreviations like “etc.” require a period.
Context: ...e (i.e. personal editor customizations, etc). 3. Run the API container ```sh ...(ETC_PERIOD)
214-216: Remove or populate empty acknowledgment linksThe acknowledgments section contains empty links that should either be removed or populated with actual references.
-* []() -* []() -* []()🧰 Tools
🪛 Markdownlint (0.35.0)
214-214: null
No empty links(MD042, no-empty-links)
215-215: null
No empty links(MD042, no-empty-links)
216-216: null
No empty links(MD042, no-empty-links)
236-236: Remove unused screenshot referenceThe
product-screenshotreference is defined but not used in the document (it's commented out). Consider removing it or uncommenting its usage.-[product-screenshot]: images/screenshot.png🧰 Tools
🪛 Markdownlint (0.35.0)
236-236: Unused link or image reference definition: "product-screenshot"
Link and image reference definitions should be needed(MD053, link-image-reference-definitions)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
.github/unraid.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
readme.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
readme.md
[style] ~13-~13: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 1881 characters long)
Context: ...contributors-shield]][contributors-url] [![Forks][forks-shield]][forks-url] [![Stargazers][stars-shield]][stars-url] [![Issues][issues-shield]][issues-url] [![MIT License][license-shield]][license-u...
(EN_EXCESSIVE_EXCLAMATION)
[style] ~128-~128: In American English, abbreviations like “etc.” require a period.
Context: ...e (i.e. personal editor customizations, etc). 3. Run the API container ```sh ...
(ETC_PERIOD)
🪛 Markdownlint (0.35.0)
readme.md
102-102: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
103-103: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
104-104: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
105-105: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
106-106: Expected: asterisk; Actual: dash
Unordered list style
(MD004, ul-style)
204-204: null
Bare URL used
(MD034, no-bare-urls)
214-214: null
No empty links
(MD042, no-empty-links)
215-215: null
No empty links
(MD042, no-empty-links)
216-216: null
No empty links
(MD042, no-empty-links)
236-236: Unused link or image reference definition: "product-screenshot"
Link and image reference definitions should be needed
(MD053, link-image-reference-definitions)
🔇 Additional comments (1)
readme.md (1)
25-27: Logo implementation looks good
The logo has been properly implemented using .github/unraid.svg, addressing the previous concern about the missing images/logo.png file.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
2 participants
Summary by CodeRabbit
Release Notes
New Features
justfilefor managing API and web projects, including commands for setup and listing available commands.Improvements
readme.mdfor better structure and clarity, including installation instructions and a table of contents..env.exampleto change the base font size and introduce a new web GUI URL.Bug Fixes