fix(dashboard): fast refusal budget when no install can run, and untangle the deps gate - #1239
Merged
Merged
Conversation
…ngle the deps gate Post-merge review of #1234 plus the Sonar complexity finding on main: - the launcher's 60s readiness budget now requires the dashboard directory to be writable: in the root-owned #1233 scenario the child refuses within a second, so the long budget only delayed the honest failure line by a minute; the writability probe moved into the shared helper so both callers see the same answer - the dependency/install gate in scripts/dashboard.js moved out of start() into ensureDashboardDeps(), resolving the cognitive complexity finding the merge introduced - the behavioral poll tests now keep the positive-case server alive on its bound port while waitForDashboard runs (no release-and-rebind race), and the helper gains an unwritable-directory case on POSIX Signed-off-by: Felipe Marzochi <fmarzochi@gmail.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Root bypasses the W_OK access probe (accessSync succeeds on a 0o555 directory under uid 0), so the case would fail in root-running Docker or CI environments; same skip family as the existing Windows guard. Signed-off-by: Felipe Marzochi <fmarzochi@gmail.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1234, addressing its post-merge review and the one new Sonar finding on main.
Summary
checkDashboardDepsnow returnswritable) so both callers see the same answerscripts/dashboard.jsmoved out ofstart()intoensureDashboardDeps(), resolving the cognitive complexity finding (S3776) the merge introduced on mainwaitForDashboardruns (no release-and-rebind race), and the helper test gains an unwritable-directory case on POSIXTesting
node --checkon the three scriptsnode tests/scripts/dashboard-launch.test.js: 6/6node tests/dashboard-server.test.js: passingSummary by cubic
Speed up honest failures when the dashboard can’t install deps and unify the writability check to avoid 60s stalls. Also simplify the start path by extracting the dependency gate to reduce complexity.
Bug Fixes
Refactors
ensureDashboardDeps()fromstart()inscripts/dashboard.jsto address Sonar cognitive complexity (S3776).checkDashboardDeps()and exposedwritable, so both the CLI and launcher use the same logic and messages for installs and failures (@egchq/egc,npm).Written for commit 2c14484. Summary will update on new commits.