🐛 Address Copilot comments: tour a11y, GPU threshold scope, coverage workflow#4186
🐛 Address Copilot comments: tour a11y, GPU threshold scope, coverage workflow#4186clubanderson merged 1 commit intomainfrom
Conversation
- Tour.tsx: Add aria-label on icon-only tour button (#4172) - ConsoleOfflineDetectionCard: Hoist GPU_CLUSTER_EXHAUSTION_THRESHOLD to module scope, fix comments "80%+" → ">80%" (#4185) - coverage-hourly.yml: Replace always() with success check to avoid noisy merge-coverage failures when shards fail (#4185) Signed-off-by: Andrew Anderson <andy@clubanderson.com>
kubestellar-prow
bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for kubestellarconsole ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
kubestellar-prow
bot
added
the
size/S
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
There was a problem hiding this comment.
Pull request overview
Addresses follow-up review feedback across UI accessibility, GPU exhaustion heuristic configuration, and CI coverage workflow gating.
Changes:
- Adds an explicit
aria-labelto the Tour trigger button for improved accessibility. - Hoists the GPU cluster exhaustion threshold constant to module scope and aligns comments with the strict
>threshold behavior. - Updates the hourly coverage workflow so the merge job runs only when the shard job reports success (and the workflow isn’t cancelled).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| web/src/components/onboarding/Tour.tsx | Adds aria-label to the Tour trigger button for screen reader support. |
| web/src/components/cards/console-missions/ConsoleOfflineDetectionCard.tsx | Hoists GPU threshold constant and clarifies “>80%” semantics in comments. |
| .github/workflows/coverage-hourly.yml | Gates the merge job on shard success/cancellation state. |
| merge-coverage: | ||
| needs: test-shard | ||
| if: always() && github.repository == 'kubestellar/console' | ||
| if: "!cancelled() && needs.test-shard.result == 'success' && github.repository == 'kubestellar/console'" | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
The new if: condition intends to only run merge-coverage when shards succeed, but test-shard currently runs Vitest with continue-on-error: true (so the shard job can still conclude success even if tests fail and no coverage is produced). As a result, needs.test-shard.result == 'success' may not actually gate on test success. Consider removing continue-on-error (matrix already has fail-fast: false so other shards will still run), and/or make the artifact upload fail when coverage is missing so the shard job correctly reports failure.
🔄 Auto-Applying Copilot Code ReviewCopilot code review found 0 code suggestion(s) and 1 general comment(s). Also address these general comments:
Push all fixes in a single commit. Run Auto-generated by copilot-review-apply workflow. |
Remove continue-on-error: true from test shard steps, which caused the test-shard job to always report success even when tests failed. The merge-coverage gate (needs.test-shard.result == 'success') was effectively a no-op. Copilot flagged this on 6 PRs (#4185, #4186, #4187, #4222, #4223, #4225). Changes: - Remove continue-on-error: true so shard failures are properly reported - Widen merge-coverage gate to !cancelled() so coverage is still merged from passing shards even when some fail - Add if-no-files-found: ignore on artifact upload so failed shards that produce no coverage file don't fail the upload step - Add final 'Fail if any shard failed' step so the overall workflow correctly surfaces test failures after merging coverage and updating the badge - Fix stale comment: '4 shards' -> '12 shards' Signed-off-by: Andrew Anderson <andy@clubanderson.com>
Remove continue-on-error: true from test shard steps, which caused the test-shard job to always report success even when tests failed. The merge-coverage gate (needs.test-shard.result == 'success') was effectively a no-op. Copilot flagged this on 6 PRs (#4185, #4186, #4187, #4222, #4223, #4225). Changes: - Remove continue-on-error: true so shard failures are properly reported - Widen merge-coverage gate to !cancelled() so coverage is still merged from passing shards even when some fail - Add if-no-files-found: ignore on artifact upload so failed shards that produce no coverage file don't fail the upload step - Add final 'Fail if any shard failed' step so the overall workflow correctly surfaces test failures after merging coverage and updating the badge - Fix stale comment: '4 shards' -> '12 shards' Signed-off-by: Andrew Anderson <andy@clubanderson.com>
Addresses comments from PRs #4172, #4185: tour button aria-label, hoisted GPU constant with fixed comments, coverage merge-job only runs on shard success.