fix(taskfile): Isolate log-viewer dependency tasks to fix race condition in webui build (fixes #2167).#2168
fix(taskfile): Isolate log-viewer dependency tasks to fix race condition in webui build (fixes #2167).#2168junhaoliao wants to merge 21 commits intoy-scope:mainfrom
Conversation
…ate` for log-viewer node_modules (fixes y-scope#2167).
junhaoliao
changed the title
deps:log-viewer and `checksum:valid…deps:log-viewer and checksum:validate for log-viewer node_modules (fixes #2167).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemoved WebUI-specific Node.js 22 build/download tasks and log-viewer node_modules checksum wiring from Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Task Runner
participant YSV as deps:yscope-log-viewer:node-modules
participant TC as toolchains:nodejs-22
participant Cleaner as clean-webui-package-node-modules
participant WebUI as webui-node-modules
participant Utils as :yscope-dev-utils:checksum
Runner->>YSV: run source -> download & extract yscope-log-viewer
YSV->>YSV: npm clean-install (uses Node 22 via TC)
YSV->>Utils: compute/store yscope node_modules checksum
YSV-->>Runner: node-modules artifact ready
Runner->>TC: ensure nodejs-22 available
TC-->>Runner: runtime/bin ready
Runner->>Cleaner: remove top-level WebUI node_modules paths
Cleaner-->>Runner: cleanup done
Runner->>WebUI: run npm clean-install (uses nodejs-22)
WebUI->>Utils: compute/store top-level webui node_modules checksum
Utils-->>WebUI: checksum written/validated
WebUI-->>Runner: webui-node-modules complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
…o `OUTPUT_DIR` for consistency.
Bill-hbrhbr
left a comment
Bill-hbrhbr
left a comment
There was a problem hiding this comment.
I think the fundamental design issue is that we should put yscope-log-viewer along with its node-modules initialization into a separate module. What I suggest is the following refactor:
- Put
nodejs22install intotaskfiles/toolchain.yaml - Move
deps:yscope-log-viewerinto a standalone task file liketaskfiles/deps/yscope-log-viewer.yamland handle the log-viewer-specific node modules initialization there. We should also do the checksum over there, rather than inside the main taskfile.
…iewer` dependencies and node_modules
…n it's useless without `sources`
… cleanup into reusable tasks
…yscope-log-viewer.yaml`
| LOG_VIEWER_OUTPUT_DIR: "{{.G_WEBUI_SRC_DIR}}/yscope-log-viewer/node_modules" | ||
| PACKAGE_OUTPUT_DIR: "{{.G_WEBUI_SRC_DIR}}/node_modules" | ||
| sources: | ||
| - "{{.G_DEPS_LOG_VIEWER_CHECKSUM_FILE}}" |
There was a problem hiding this comment.
Maybe we should put this back? Will there be cases where log viewer source code changes, but nothing affects the node modules and its checksum.
There was a problem hiding this comment.
right. let me double check and add it back
There was a problem hiding this comment.
actually i remember this is intentional. webui-node-modules should only be responsible for ensuring the webui components's node_modules are cleanly built.
do note that the log-viewer source code is consumed at
- task
webui, which depends on - task webui-node-modules, which depends on
- task
deps:yscope-log-viewer:node-modules, which depends on - task
deps:yscope-log-viewer:node-modules-checksum-validate, which depends on
deps:yscope-log-viewer:source.
we have seen that
webuiincludesG_WEBUI_PACKAGE_NODE_MODULES_CHECKSUM_FILEin its sourceswebui-node-modulesincludesG_WEBUI_LOG_VIEWER_NODE_MODULES_CHECKSUM_FILEin its sources, and generatesG_WEBUI_PACKAGE_NODE_MODULES_CHECKSUM_FILEdeps:yscope-log-viewer:node-modulesincludesG_DEPS_LOG_VIEWER_CHECKSUM_FILEin its sources, and generatesG_WEBUI_LOG_VIEWER_NODE_MODULES_CHECKSUM_FILEsourcegeneratesG_DEPS_LOG_VIEWER_CHECKSUM_FILE
So any change to the log viewer source already propagates through the checksum chain. Additionally, webui has **/* (excluding node_modules and dist) in its sources, which directly catches log viewer source file changes.
Now i think about it, maybe we should move the log-viewer building to deps/yscope-log-viewer.yaml so the concerns will be cleanly isolated. let me try making the changes now
…code review Co-authored-by: Bingran Hu <bingran.hu@yscope.com>
Co-authored-by: Bingran Hu <bingran.hu@yscope.com>
…de review Co-authored-by: Bingran Hu <bingran.hu@yscope.com>
Co-authored-by: Bingran Hu <bingran.hu@yscope.com>
…`yscope-log-viewer.yaml`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@taskfile.yaml`:
- Around line 99-103: The clean-webui task currently only removes node_modules
(via subtasks clean-webui-package-node-modules and
deps:yscope-log-viewer:clean-node-modules) but no longer deletes the built web
UI output or checksum files; update the clean-webui task to also remove the
build output directory ({{.G_WEBUI_BUILD_DIR}}) and any webui checksum files so
the task truly provides a clean rebuild boundary—add steps after the existing
cmds that delete {{.G_WEBUI_BUILD_DIR}} (rm -rf style) and remove the webui
checksum files (the checksum filename pattern used by the repo) so both
artifacts are cleaned when running task clean-webui.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f95a07c5-0c1e-4ad1-9e26-0a52276ba439
📒 Files selected for processing (2)
taskfile.yamltaskfiles/deps/yscope-log-viewer.yaml
taskfile.yaml
Outdated
| clean-webui: | ||
| cmds: | ||
| - task: "clean-webui-package-node-modules" | ||
| - task: "deps:yscope-log-viewer:clean-node-modules" | ||
|
|
There was a problem hiding this comment.
clean-webui no longer removes the built webui artefacts.
After this change, task clean-webui only deletes node_modules. {{.G_WEBUI_BUILD_DIR}} and the webui checksum files stay behind, so the task no longer gives you a clean webui rebuild boundary on its own.
Suggested fix
clean-webui:
cmds:
+ - "rm -rf '{{.G_WEBUI_BUILD_DIR}}'"
+ - "rm -f '{{.G_WEBUI_CHECKSUM_FILE}}' '{{.G_WEBUI_PACKAGE_NODE_MODULES_CHECKSUM_FILE}}'"
- task: "clean-webui-package-node-modules"
- task: "deps:yscope-log-viewer:clean-node-modules"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@taskfile.yaml` around lines 99 - 103, The clean-webui task currently only
removes node_modules (via subtasks clean-webui-package-node-modules and
deps:yscope-log-viewer:clean-node-modules) but no longer deletes the built web
UI output or checksum files; update the clean-webui task to also remove the
build output directory ({{.G_WEBUI_BUILD_DIR}}) and any webui checksum files so
the task truly provides a clean rebuild boundary—add steps after the existing
cmds that delete {{.G_WEBUI_BUILD_DIR}} (rm -rf style) and remove the webui
checksum files (the checksum filename pattern used by the repo) so both
artifacts are cleaned when running task clean-webui.
… to `log-viewer` for consistency
…y removing redundant `yscope-log-viewer` dependencies
junhaoliao
changed the title
deps:log-viewer and checksum:validate for log-viewer node_modules (fixes #2167).…ecific package.json files
…`log-viewer.yaml`
2 participants
Description
Isolate the log-viewer's source download,
node_modulesinstallation, and production build into aself-contained
deps:yscope-log-viewertaskfile (taskfiles/deps/log-viewer.yaml). This fixes arace condition where
download-and-extract-tar(which doesrm -rfon the source directory) couldrun concurrently with
checksum:validatefor the log-viewer'snode_modules, causing intermittentbuild failures (fixes #2167).
Key changes:
taskfiles/deps/log-viewer.yaml: Containssource,node-modules, andbuildtaskswith proper dependency ordering.
node-modules-checksum-validateensuresdownload-and-extract-tarcompletes beforenode_modulesis validated (eliminating the race).The
buildtask recomputes checksums after vite's build to account fornode_modules/.vite-temp.nodejs-22totaskfiles/toolchains.yaml: Useyscope-dev-utils'sdownload-and-extract-tarwith pinned per-platform SHA256 hashes, replacing the olddownload-and-extract-tarandnodejstasks intaskfile.yaml.webui-node-modulesfrom log-viewer: The webui's node-modules task no longerinstalls or checksums the log-viewer's
node_modules; it only manages the webui package's ownworkspace dependencies.
clean-webui: Only cleans the webui package'snode_modulesand build output; thelog-viewer's cleanup is handled by its own
clean-node-modulestask.Checklist
breaking change.
Validation performed
1.
deps:yscope-log-viewercaching after clean buildVerifies that after
rm -rf .task/ build/, the second run is fully cached (no re-extraction,no re-install, no rebuild).
Commands:
Run 2 output:
2.
webuibuild succeeds and caches correctlyVerifies the full
webuipipeline builds from a clean state and that log-viewer tasks are cachedon the second run.
Commands:
Run 2 output (log-viewer tasks):
Note:
webui-node-modulesre-runs on the second invocation. This is a pre-existing issue(confirmed on
main) unrelated to this PR.3. YAML lint passes
Command:
Output:
No errors.
Summary by CodeRabbit