feat: Match deployment layout to URL-structure so the Fastify server can be replaced with a simple server.#54
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughReplaces the custom Node.js Fastify server with a generic http-server workflow, updates deployment docs to reference "webserve", changes per-project build layout to nested Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Download as scripts/download-projects.py
participant Build as build-main-docs
participant Symlink as symlink-project-docs
participant Server as http-server (webserve)
Dev->>Download: run download-projects (reads conf/projects.json)
Download-->>Dev: clones repos to build/project-docs/<project>/<version>
Dev->>Build: run build-main-docs (generate main site to G_MAIN_DOCS_OUTPUT_DIR)
Build-->>Dev: outputs main docs to G_MAIN_DOCS_OUTPUT_DIR
Dev->>Symlink: run symlink-project-docs (reads G_PROJECTS_DOCS_DIR)
Symlink-->>Server: creates symlinks under G_PUBLISHABLE_OUTPUT_DIR
Dev->>Server: run serve (starts http-server serving G_PUBLISHABLE_OUTPUT_DIR)
Server-->>Dev: serves sites at /<project>/<version>/ prefixes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
scripts/download-projects.py (2)
154-155:⚠️ Potential issue | 🔴 Critical
rmdir()will fail on non-empty directories.When
force_overwriteis true,repo_dir.rmdir()is called to remove a cloned repository directory. However,Path.rmdir()only removes empty directories and will raiseOSErrorif the directory contains files (which a cloned repo always will).Use
shutil.rmtree()instead.🐛 Proposed fix
Add the import at the top of the file:
import subprocess +import shutil from pathlib import PathThen fix the removal logic:
if force_overwrite: - repo_dir.rmdir() + shutil.rmtree(repo_dir) else:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/download-projects.py` around lines 154 - 155, The code calls repo_dir.rmdir() when force_overwrite is true but Path.rmdir() fails on non-empty dirs; import shutil at the top and replace repo_dir.rmdir() with shutil.rmtree(repo_dir) (optionally guard with repo_dir.exists() to avoid exceptions) so the cloned repository is removed recursively; ensure the change is applied where force_overwrite and repo_dir are used in the download logic.
98-101:⚠️ Potential issue | 🟡 MinorUnreachable code after
check=True.When
check=Trueis passed tosubprocess.run(), it raisesCalledProcessErroron non-zero return codes. The subsequent checkif 0 != proc.returncodeis unreachable because the exception would have already been raised.Either remove
check=Trueand handle the error manually, or remove the unreachable return code check.🐛 Proposed fix (Option 1: Remove check=True)
- proc = subprocess.run(cmd, cwd=repo_dir, stdout=subprocess.PIPE, check=True) + proc = subprocess.run(cmd, cwd=repo_dir, stdout=subprocess.PIPE) if 0 != proc.returncode: raise RuntimeError(f"Failed to get commit hash for {commit} in {repo_dir}.")🐛 Proposed fix (Option 2: Remove manual check)
- proc = subprocess.run(cmd, cwd=repo_dir, stdout=subprocess.PIPE, check=True) - if 0 != proc.returncode: - raise RuntimeError(f"Failed to get commit hash for {commit} in {repo_dir}.") + try: + proc = subprocess.run(cmd, cwd=repo_dir, stdout=subprocess.PIPE, check=True) + except subprocess.CalledProcessError as e: + raise RuntimeError(f"Failed to get commit hash for {commit} in {repo_dir}.") from e return proc.stdout.decode("utf-8").strip()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/download-projects.py` around lines 98 - 101, The code calls subprocess.run(cmd, cwd=repo_dir, stdout=subprocess.PIPE, check=True) and then checks proc.returncode, but that check is unreachable; remove the manual check and just return proc.stdout.decode(...).strip() (i.e., delete the if 0 != proc.returncode block) in the function where subprocess.run is used (referencing variables/proc, cmd, commit, repo_dir) so errors are handled by subprocess.run's check=True behavior.docs/dev-guide/misc-deploying.md (1)
39-43:⚠️ Potential issue | 🟡 MinorPath format is inconsistent with the new directory structure.
The PR changes the directory structure from flat (
<project>-<version>) to nested (<project>/<version>). This documentation still references the old flat format.📝 Proposed fix
3. Build the docs for each project and version in `conf/projects.json`: ```shell - cd build/<project>-<version> + cd build/project-docs/<project>/<version> task docs:site - cd ../ + cd ../../.. ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/dev-guide/misc-deploying.md` around lines 39 - 43, Update the shell example to use the new nested directory layout by replacing the initial cd into the old flat build/<project>-<version> with cd into build/project-docs/<project>/<version>, then run the same task docs:site and update the subsequent cd ../ to climb back out to the repo root (cd ../../..) so the navigation matches the new structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/dev-guide/misc-deploying.md`:
- Around line 20-21: The documentation incorrectly references a shell script;
update the sentence that mentions scripts/download-projects.sh to reference the
actual Python script scripts/download-projects.py and ensure the surrounding
text still points to conf/projects.json for repo/version downloads so readers
run the correct script (look for the phrase mentioning download-projects.sh in
the docs/dev-guide/misc-deploying.md and replace with download-projects.py).
In `@taskfiles/docs.yaml`:
- Around line 91-92: The npm install command currently installs http-server
without a version ("npm --prefix '{{.OUTPUT_DIR}}' install http-server"); change
it to pin the package to a stable major (e.g., "http-server@14" or exact
"http-server@14.1.1") to ensure reproducible builds—update the second command in
the snippet to "npm --prefix '{{.OUTPUT_DIR}}' install http-server@14" (or
`@14.1.1` if you prefer exact pinning).
---
Outside diff comments:
In `@docs/dev-guide/misc-deploying.md`:
- Around line 39-43: Update the shell example to use the new nested directory
layout by replacing the initial cd into the old flat build/<project>-<version>
with cd into build/project-docs/<project>/<version>, then run the same task
docs:site and update the subsequent cd ../ to climb back out to the repo root
(cd ../../..) so the navigation matches the new structure.
In `@scripts/download-projects.py`:
- Around line 154-155: The code calls repo_dir.rmdir() when force_overwrite is
true but Path.rmdir() fails on non-empty dirs; import shutil at the top and
replace repo_dir.rmdir() with shutil.rmtree(repo_dir) (optionally guard with
repo_dir.exists() to avoid exceptions) so the cloned repository is removed
recursively; ensure the change is applied where force_overwrite and repo_dir are
used in the download logic.
- Around line 98-101: The code calls subprocess.run(cmd, cwd=repo_dir,
stdout=subprocess.PIPE, check=True) and then checks proc.returncode, but that
check is unreachable; remove the manual check and just return
proc.stdout.decode(...).strip() (i.e., delete the if 0 != proc.returncode block)
in the function where subprocess.run is used (referencing variables/proc, cmd,
commit, repo_dir) so errors are handled by subprocess.run's check=True behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
docs/dev-guide/misc-deploying.mdscripts/download-projects.pyserver/.envserver/.gitignoreserver/README.mdserver/package.jsonserver/src/main.jsserver/src/routes.jstaskfiles/docs.yaml
💤 Files with no reviewable changes (6)
- server/.env
- server/src/main.js
- server/.gitignore
- server/README.md
- server/package.json
- server/src/routes.js
| - "rm -rf '{{.OUTPUT_DIR}}'" | ||
| - |- | ||
| cd "{{.G_SOURCE_DIR}}" | ||
| npm install | ||
| - "npm --prefix '{{.OUTPUT_DIR}}' install http-server" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
npm http-server latest stable version package
💡 Result:
The latest stable (current latest dist-tag) version of the npm package http-server is 14.1.1. [1][2]
Consider pinning the http-server version.
Installing http-server without a version specification may lead to non-reproducible builds if the package introduces breaking changes. The latest stable version is 14.1.1; pinning to http-server@14 allows patch updates while preventing major version breaks.
♻️ Proposed fix
- "rm -rf '{{.OUTPUT_DIR}}'"
- - "npm --prefix '{{.OUTPUT_DIR}}' install http-server"
+ - "npm --prefix '{{.OUTPUT_DIR}}' install http-server@14"📝 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.
| - "rm -rf '{{.OUTPUT_DIR}}'" | |
| - |- | |
| cd "{{.G_SOURCE_DIR}}" | |
| npm install | |
| - "npm --prefix '{{.OUTPUT_DIR}}' install http-server" | |
| - "rm -rf '{{.OUTPUT_DIR}}'" | |
| - "npm --prefix '{{.OUTPUT_DIR}}' install http-server@14" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@taskfiles/docs.yaml` around lines 91 - 92, The npm install command currently
installs http-server without a version ("npm --prefix '{{.OUTPUT_DIR}}' install
http-server"); change it to pin the package to a stable major (e.g.,
"http-server@14" or exact "http-server@14.1.1") to ensure reproducible
builds—update the second command in the snippet to "npm --prefix
'{{.OUTPUT_DIR}}' install http-server@14" (or `@14.1.1` if you prefer exact
pinning).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/dev-guide/misc-deploying.md`:
- Line 53: Replace the vague phrase in the step "Move the extracted content to
wherever the webserve expects it to be." with an explicit destination: either a
concrete filesystem path (e.g., /var/www/site or /usr/share/nginx/html) or the
exact configuration key/name that controls the webserver document root (e.g.,
nginx root in server block or the webserve.config "document_root" field). Update
the line to state the exact path or config key and, if applicable, show the
command or variable to verify that destination so operators have an unambiguous
deploy target.
- Around line 40-43: The current step that "cd"s into the per-project/version
build directory and then uses a relative "cd ../" to return is fragile; update
the workflow so after entering the project/version build dir you return to a
deterministic location instead of "cd ../" — for example use "cd -" to go back
to the previous working directory or change to a stable repo-root variable (set
once before the loop) and use that to reset; ensure the repo-root variable is
defined prior to calling the per-project build step and replace the relative "cd
../" reference with the deterministic return.
docs/dev-guide/misc-deploying.md
Outdated
| The tar will be written to `build/yscope-docs-release.tar.gz`. | ||
| 5. Upload the tar to the deployment server and untar it. | ||
| 6. Follow the instructions in `yscope-docs-release/server/README.md` to start the server. | ||
| 6. Move the extracted content to wherever the webserve expects it to be. |
There was a problem hiding this comment.
Replace vague deploy target with a concrete path or config key.
“wherever the webserve expects it” is too ambiguous for deployment instructions. Please document the exact destination path (or the exact config field that defines it) to avoid operator drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/dev-guide/misc-deploying.md` at line 53, Replace the vague phrase in the
step "Move the extracted content to wherever the webserve expects it to be."
with an explicit destination: either a concrete filesystem path (e.g.,
/var/www/site or /usr/share/nginx/html) or the exact configuration key/name that
controls the webserver document root (e.g., nginx root in server block or the
webserve.config "document_root" field). Update the line to state the exact path
or config key and, if applicable, show the command or variable to verify that
destination so operators have an unambiguous deploy target.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/dev-guide/misc-deploying.md (2)
40-42: 🧹 Nitpick | 🔵 TrivialGood improvement using
cd -for deterministic directory reset.The change from
cd ../tocd -addresses the previous concern about fragile relative paths. This now reliably returns to the previous working directory.Optional: Consider clarifying the iteration process.
While the step mentions "for each project and version in
conf/projects.json", the commands shown require manual substitution of<project>and<version>placeholders. Consider adding a note that this step must be repeated for each entry, or provide a shell loop example.📋 Example shell loop for clarity
For each project and version listed in `conf/projects.json`, run: ```shell cd build/project-docs/<project>/<version> task docs:site cd -Or use a loop (example for bash):
# Assuming you've extracted projects/versions from conf/projects.json for project_version in clp/main clp/v0.1.0 clp-ffi-py/main; do cd "build/project-docs/${project_version}" task docs:site cd - done</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/dev-guide/misc-deploying.mdaround lines 40 - 42, Update the
documentation to clarify that the three commands (cd
build/project-docs//, task docs:site, cd -) must be repeated
for every entry in conf/projects.json; either add a short note after the
commands stating "Repeat for each project/version in conf/projects.json" or
include a compact shell loop example demonstrating iterating over
project/version pairs and running the sequence, referencing the placeholders
and and the conf/projects.json source so readers know to
substitute or iterate.</details> --- `53-53`: _⚠️ Potential issue_ | _🟡 Minor_ **Specify the concrete deployment destination.** The instruction "wherever the webserver expects it to be" remains too ambiguous for reliable deployment. Please document the exact destination path (e.g., `/var/www/yscope-docs`, `/usr/share/nginx/html`) or reference the specific configuration field that controls the document root. <details> <summary>📝 Example improvements</summary> Option 1 - Concrete path: ```diff -6. Move the extracted content to wherever the webserver expects it to be. +6. Move the extracted content to the web server's document root (e.g., `/var/www/yscope-docs` or as configured in your web server).Option 2 - Reference configuration:
-6. Move the extracted content to wherever the webserver expects it to be. +6. Move the extracted content to the document root specified in your web server configuration (e.g., the `root` directive in nginx or `DocumentRoot` in Apache).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/dev-guide/misc-deploying.md` at line 53, Replace the ambiguous step "Move the extracted content to wherever the webserver expects it to be." with a concrete destination or a config reference: either specify an example path (e.g., /var/www/yscope-docs or /usr/share/nginx/html) and any required ownership/permissions, or reference the exact configuration field to update (e.g., nginx "root" in the server block or Apache "DocumentRoot") and include a short instruction to update that field and reload the webserver (mention the original sentence to replace).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/dev-guide/misc-deploying.md`:
- Line 5: Replace the non-standard term "webserve" with the standard term "web
server" (two words) throughout the document; search for the token "webserve"
(e.g., in the phrase "use a single webserve to serve each site") and update it
to "web server" for clarity and consistency, also scan for other occurrences and
adjust surrounding grammar if needed (accept "webserver" only if you prefer a
single-word form consistently).
---
Duplicate comments:
In `@docs/dev-guide/misc-deploying.md`:
- Around line 40-42: Update the documentation to clarify that the three commands
(cd build/project-docs/<project>/<version>, task docs:site, cd -) must be
repeated for every entry in conf/projects.json; either add a short note after
the commands stating "Repeat for each project/version in conf/projects.json" or
include a compact shell loop example demonstrating iterating over
project/version pairs and running the sequence, referencing the placeholders
<project> and <version> and the conf/projects.json source so readers know to
substitute or iterate.
- Line 53: Replace the ambiguous step "Move the extracted content to wherever
the webserver expects it to be." with a concrete destination or a config
reference: either specify an example path (e.g., /var/www/yscope-docs or
/usr/share/nginx/html) and any required ownership/permissions, or reference the
exact configuration field to update (e.g., nginx "root" in the server block or
Apache "DocumentRoot") and include a short instruction to update that field and
reload the webserver (mention the original sentence to replace).
AVMatthews
left a comment
AVMatthews
left a comment
There was a problem hiding this comment.
Generally looks good. Mostly I have some comments about the explanation text. I also tested a combined version of this PR and PR #52 and it should work just fine with some additions to the task file.
| Each project and its versions should be listed in `conf/projects.json` so that | ||
| `scripts/download-projects.py` can download the repos and check out the correct versions. | ||
|
|
||
| ## Step-by-step guide |
There was a problem hiding this comment.
Maybe add a note here about needing correct build environment ... something like "Note: Please refer to relevant repo for build environment setup and requirements"
| cd build/project-docs/<project>/<version> | ||
| task docs:site | ||
| cd ../ | ||
| cd - |
There was a problem hiding this comment.
I think we could cut this line. I think it is clear what needs to be done for each, but we don't know what directory they may be coming from or going back to.
| @@ -50,9 +37,9 @@ directory's path to generate routes similar to the previous table. | |||
| 3. Build the docs for each project and version in `conf/projects.json`: | |||
There was a problem hiding this comment.
Similar to the note about the note at the top of the section we could add a nod to checking the relevant repo for any specific build instructions or environment set up.
| to use a single `node` process to serve each site at a different URL prefix (e.g., CLP at | ||
| `/clp/main` and clp-ffi-py at `/clp-ffi-py/main`). We also want the layout on disk to be predictable | ||
| so that we can use a simple config file to configure all the sites that need to be served. | ||
| to use a single webserver to serve each site at a different URL prefix (e.g., CLP at `/clp/main` and |
There was a problem hiding this comment.
To make this more readable we could do something more like ...
Goals:
- Use a single webserver to serve each site at a different URL prefix (e.g., CLP at
/clp/mainand clp-ffi-py at/clp-ffi-py/main). - Use predictable on-disk layout so a simple config file can be used to configure all the sites that need to be served.
| clp-ffi-py at `/clp-ffi-py/main`). We also want the layout on disk to be predictable so that we can | ||
| use a simple config file to configure all the sites that need to be served. | ||
|
|
||
| One approach would be to build all the sites using Task and then assemble them into a single |
There was a problem hiding this comment.
Simplify?
Each site (and version of docs) may have different build dependencies requiring unique build environments. This prevent us from building and assembling all the sites together using Task. Instead, we build all sites and variants individually before assembling them into a single deployment.
In the future, we are plan to have a GitHub workflow for each repo to build the site in a container and then publishes it directly to docs.yscope.com at the expected location.
2 participants
Description
The current layout of the built HTML files looks like:
build/htmlbuild/clp-main/build/docs/htmlbuild/clp-v0.9.0/build/docs/htmlMeanwhile, we want the main site and project (e.g., clp) sites to be available at:
//clp/main/clp/v0.9.0This means we needed some kind of routing to serve, for example,
/clp/mainfrom/build/clp-main/build/docs/html. In hindsight, this was kind of unnecessary since we can simply create and symlink each project'shtmldirectory intobuild/html/<project>/<version>.Thus, this PR does that and gets rid of the Fastify server entirely. More specifically, this PR:
build/project-docs/<project>/<version>build/main-docs/buildtask now builds the main docs site as a subtask, copies it tobuild/html, and then symlinks the built project docs into their respective positions inbuild/html.docs:release-tartask now simply tarsbuild/htmlservetask now uses a simplehttp-server(where thehttp-servertask is largely copied from clp).Checklist
breaking change.
Validation performed
task cleantask docs:buildtask docs:serveand validated the output in the browser.task docs:download-projectscd project-docs/<project>/<version> && task docs:sitetask docs:serveand validated the output in the browser.task docs:release-tarnpx http-server htmlSummary by CodeRabbit
Documentation
Refactor
Chores