🐛 Fix 5 bugs: Falco demo, screenshot preview, deployment mismatch, ArgoCD demo, cluster reachability#4204
Conversation
…mismatch, ArgoCD demo mode, cluster reachability - #4198: FalcoAlerts now gates demo content on isDemoMode instead of always showing hardcoded demo alerts. When not in demo mode, shows install prompt and empty state. - #4199: Add Eye/preview button to screenshot thumbnails in the feedback modal. Clicking opens a fullscreen image overlay (dismissible via Escape, close button, or backdrop click). - #4200: DeploymentDrillDown now reconciles reason/message with live kubectl data. When live fetch shows healthy (readyReplicas === replicas), stale failure reason/message is cleared. When still unhealthy, reason is derived from deployment conditions. - #4201: ArgoCD hooks no longer force mock fallback when API returns empty items or zero totals with isDemoData=false. Integration banner in all three ArgoCD cards is now conditional on isDemoData. - #4202: Add external API server TCP probe in Go backend's GetClusterHealth. New externallyReachable field distinguishes internal vs external reachability. ClusterHealth card shows warning badge when cluster is internally healthy but externally unreachable. Closes #4198, closes #4199, closes #4200, closes #4201, closes #4202 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. |
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
kubestellar-prow
bot
added
the
size/L
|
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
This PR batches fixes for five console bugs across GitOps (ArgoCD), compliance (Falco demo gating), feedback UX (screenshot preview), deployment drilldown status reconciliation, and cluster reachability (external TCP probe surfaced to UI).
Changes:
- Adjust ArgoCD hooks/cards to avoid incorrect demo fallback and hide integration banners when live (non-demo) data is in use.
- Add a screenshot preview action in the FeatureRequest modal and reconcile stale deployment failure reason/message in the drilldown after live refresh.
- Introduce an external API server TCP probe in the Go backend, plumb
externallyReachablethrough MCP types, and show an “ext. unreachable” warning in ClusterHealth.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/k8s/client.go | Adds external TCP probe (ExternallyReachable) during cluster health and appends an issue when probe fails. |
| web/src/hooks/useArgoCD.ts | Treats empty/zero live responses as valid live data (no forced mock fallback). |
| web/src/hooks/mcp/types.ts | Extends MCP cluster types with externallyReachable. |
| web/src/hooks/mcp/shared.ts | Passes externallyReachable from backend health into cached cluster state. |
| web/src/components/cards/ComplianceCards.tsx | Gates Falco demo alerts behind isDemoMode, shows install/empty state otherwise. |
| web/src/components/cards/ArgoCDApplications.tsx | Shows ArgoCD integration notice only in demo/fallback mode. |
| web/src/components/cards/ArgoCDHealth.tsx | Shows ArgoCD integration notice only in demo/fallback mode. |
| web/src/components/cards/ArgoCDSyncStatus.tsx | Shows ArgoCD integration notice only in demo/fallback mode. |
| web/src/components/cards/ClusterHealth.tsx | Adds “ext. unreachable” warning badge when external probe fails. |
| web/src/components/drilldown/views/DeploymentDrillDown.tsx | Reconciles/clears stale reason/message based on live kubectl deployment JSON. |
| web/src/components/feedback/FeatureRequestModal.tsx | Adds screenshot thumbnail preview button and fullscreen image overlay. |
| <div | ||
| ref={screenshotPreviewRef} | ||
| className="fixed inset-0 z-[100] flex items-center justify-center bg-black/70 backdrop-blur-sm" | ||
| onClick={(e) => { | ||
| if (e.target === screenshotPreviewRef.current) { | ||
| setPreviewImageSrc(null) | ||
| } | ||
| }} | ||
| onKeyDown={(e) => { | ||
| if (e.key === 'Escape') { | ||
| setPreviewImageSrc(null) | ||
| } | ||
| }} |
There was a problem hiding this comment.
The screenshot preview overlay’s Escape handling likely won’t work: the div isn’t focusable (so onKeyDown won’t fire reliably), and BaseModal’s global Escape handler will close the entire modal instead of just the preview. Consider adding a document-level keydown listener while previewImageSrc is set (like the existing isPreviewFullscreen logic) and/or disabling BaseModal closeOnEscape while the preview is open so Escape closes the overlay first.
| <button | ||
| type="button" | ||
| onClick={e => { e.stopPropagation(); setPreviewImageSrc(s.preview) }} | ||
| className="p-1.5 rounded-md bg-secondary/80 text-foreground hover:bg-secondary transition-colors" | ||
| title="Preview screenshot" | ||
| aria-label="Preview screenshot" | ||
| > | ||
| <Eye className="w-3.5 h-3.5" /> | ||
| </button> |
There was a problem hiding this comment.
New screenshot preview behavior isn’t covered by tests. web/src/components/feedback/FeatureRequestModal.test.tsx currently only asserts the export; please add RTL/Vitest tests for showing the preview button per thumbnail and opening/closing the overlay (backdrop, close button, Escape) to prevent regressions.
| {/* Warn when cluster is internally reachable but API server is externally unreachable (#4202) */} | ||
| {!clusterUnreachable && !clusterLoading && cluster.externallyReachable === false && ( | ||
| <span | ||
| className="flex items-center gap-0.5 text-2xs px-1.5 py-0.5 rounded bg-yellow-500/15 text-yellow-400 border border-yellow-500/20 shrink-0" | ||
| title="API server externally unreachable — cluster healthy internally but external access may be blocked" | ||
| > | ||
| <WifiOff className="w-3 h-3" /> | ||
| <span className="hidden sm:inline">ext. unreachable</span> | ||
| </span> |
There was a problem hiding this comment.
This externally-reachable warning badge can become icon-only on small screens (the text is hidden). A title tooltip isn’t announced by screen readers; add an aria-label or an sr-only text node so the warning is accessible when only the icon is visible.
| // Falco has no live data hook yet so isDemoData is always true. | ||
| // However, we still gate the demo content on isDemoMode so the card | ||
| // correctly shows an empty / install-prompt state when a real agent | ||
| // is connected but Falco is not installed. |
There was a problem hiding this comment.
The comment says Falco “isDemoData is always true”, but the code reports isDemoData: showDemo (false when not in demo mode). Please update the comment to match the actual behavior (demo alerts gated by isDemoMode; otherwise not demo).
| // Falco has no live data hook yet so isDemoData is always true. | |
| // However, we still gate the demo content on isDemoMode so the card | |
| // correctly shows an empty / install-prompt state when a real agent | |
| // is connected but Falco is not installed. | |
| // Falco has no live data hook yet, so this card only shows demo alerts. | |
| // Demo content (and isDemoData) is gated by isDemoMode; when not in | |
| // demo mode the card shows an empty / install-prompt state so it | |
| // behaves correctly when a real agent is connected but Falco itself | |
| // is not installed. |
| // Reconcile reason/message with live state (#4200): | ||
| // If live data shows healthy, clear the stale failure reason/message. | ||
| // If still unhealthy, derive reason from deployment conditions. | ||
| if (liveReady === liveReplicas && liveReplicas > 0) { | ||
| setLiveReason(undefined) | ||
| setLiveMessage(undefined) | ||
| } else { | ||
| // Extract current condition from the deployment status | ||
| const conditions = (deploy.status?.conditions || []) as Array<{ type: string; status: string; reason?: string; message?: string }> | ||
| const failedCondition = conditions.find( | ||
| (c: { type: string; status: string }) => | ||
| (c.type === 'Available' && c.status === 'False') || | ||
| (c.type === 'Progressing' && c.status === 'False') || | ||
| (c.type === 'ReplicaFailure' && c.status === 'True') | ||
| ) | ||
| if (failedCondition) { | ||
| setLiveReason(failedCondition.reason || liveReason) | ||
| setLiveMessage(failedCondition.message || liveMessage) | ||
| } |
There was a problem hiding this comment.
In fetchData(), setLiveReason/setLiveMessage use the captured liveReason/liveMessage values, which can be stale by the time the async kubectl call resolves. Prefer functional state updates (prev => ...) or fall back to the initial payload values. Also, when liveReplicas/liveReady are both 0 (scaled to zero), the code won’t clear a stale failure reason/message, which can still show “Degraded” with an old failure reason.
| if (!result.isDemoData) { | ||
| // Real data from ArgoCD — use it even if empty (ArgoCD installed | ||
| // but no applications deployed yet). Previously an empty list | ||
| // was treated as "no ArgoCD" which triggered mock fallback (#4201). | ||
| setApplications(result.items || []) | ||
| setIsDemoData(false) | ||
| setError(null) | ||
| setConsecutiveFailures(0) | ||
| setLastRefresh(Date.now()) | ||
| initialLoadDone.current = true | ||
| saveToCache(APPS_CACHE_KEY, result.items, false) | ||
| saveToCache(APPS_CACHE_KEY, result.items || [], false) | ||
| return |
There was a problem hiding this comment.
Hook semantics changed to treat an empty applications list as live data (isDemoData=false). Please extend web/src/hooks/tests/useArgoCD.test.ts to cover the new behavior (API returns isDemoData=false with items=[] should not fall back to mock/demo) so #4201 stays fixed.
| {/* Integration notice — only shown in demo/fallback mode (#4201) */} | ||
| {isDemoData && ( | ||
| <div className="flex items-start gap-2 p-2 mb-3 rounded-lg bg-orange-500/10 border border-orange-500/20 text-xs"> | ||
| <AlertCircle className="w-4 h-4 text-orange-400 flex-shrink-0 mt-0.5" /> | ||
| <div> | ||
| <p className="text-orange-400 font-medium">{t('argoCDHealth.argocdIntegration')}</p> | ||
| <p className="text-muted-foreground"> | ||
| {t('argoCDHealth.installArgoCD')}{' '} | ||
| <a href="https://argo-cd.readthedocs.io/en/stable/getting_started/" target="_blank" rel="noopener noreferrer" className="text-purple-400 hover:underline inline-block py-2"> | ||
| {t('argoCDHealth.installGuide')} | ||
| </a> | ||
| </p> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
ArgoCDHealth now shows the integration banner only when isDemoData is true, but the existing unit test currently asserts the banner is rendered in the live-data scenario. Update the tests (web/src/components/cards/tests/ArgoCDHealth.test.tsx) to match the new conditional behavior.
| {/* Integration notice — only shown in demo/fallback mode (#4201) */} | ||
| {isDemoData && ( | ||
| <div className="flex items-start gap-2 p-2 mb-3 rounded-lg bg-orange-500/10 border border-orange-500/20 text-xs"> | ||
| <AlertCircle className="w-4 h-4 text-orange-400 flex-shrink-0 mt-0.5" /> | ||
| <div> | ||
| <p className="text-orange-400 font-medium">{t('argoCDSyncStatus.argocdIntegration')}</p> | ||
| <p className="text-muted-foreground"> | ||
| {t('argoCDSyncStatus.installArgoCD')}{' '} | ||
| <a href="https://argo-cd.readthedocs.io/en/stable/getting_started/" target="_blank" rel="noopener noreferrer" className="text-purple-400 hover:underline inline-block py-2"> | ||
| {t('argoCDSyncStatus.installGuide')} | ||
| </a> | ||
| </p> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
ArgoCDSyncStatus now gates the integration banner behind isDemoData, but the existing unit test asserts the banner in the live-data scenario. Update web/src/components/cards/tests/ArgoCDSyncStatus.test.tsx accordingly so CI doesn’t fail.
| // Populate the API server URL from the REST config for the frontend to display. | ||
| // Also run an external TCP probe to distinguish internal-only vs external reachability (#4202). | ||
| if health.Reachable { | ||
| m.mu.RLock() | ||
| cfg := m.configs[contextName] | ||
| m.mu.RUnlock() | ||
| if cfg != nil && cfg.Host != "" { | ||
| health.APIServer = cfg.Host | ||
| reachable := probeAPIServer(cfg.Host) | ||
| health.ExternallyReachable = &reachable | ||
| if !reachable { | ||
| health.Issues = append(health.Issues, "API server externally unreachable (TCP probe failed)") | ||
| } |
There was a problem hiding this comment.
New externallyReachable behavior isn’t unit-tested. pkg/k8s/client_test.go has GetClusterHealth coverage; consider adding a test that sets m.configs[contextName].Host to a local listener address and asserts ExternallyReachable is set true/false and that the “API server externally unreachable” issue is appended when the TCP probe fails.
| // Real data from ArgoCD — use it even if totals are zero | ||
| // (ArgoCD installed but no applications yet). Previously a | ||
| // zero total was treated as "no ArgoCD" which triggered mock | ||
| // fallback even when ArgoCD was running (#4201). | ||
| setStats(result.stats) | ||
| setIsDemoData(false) | ||
| setError(null) | ||
| setConsecutiveFailures(0) | ||
| setLastRefresh(Date.now()) | ||
| initialLoadDone.current = true | ||
| saveToCache(HEALTH_CACHE_KEY, result.stats, false) | ||
| return |
There was a problem hiding this comment.
Health/sync hooks now treat zero-total stats as live data (isDemoData=false) instead of forcing mock fallback. Please add/adjust hook tests to cover the “isDemoData=false with all-zero stats” case for useArgoCDHealth and useArgoCDSyncStatus so this regression doesn’t reappear.
🔄 Auto-Applying Copilot Code ReviewCopilot code review found 1 code suggestion(s) and 10 general comment(s). @copilot Please apply all of the following code review suggestions:
Also address these general comments:
Push all fixes in a single commit. Run Auto-generated by copilot-review-apply workflow. |
Summary
Fixes 5 bug issues in a single batch:
Falco Alerts shows demo data even when local agent is running #4198 - Falco Alerts shows demo data even when local agent is running: FalcoAlerts now gates demo content on
isDemoModeinstead of always showing hardcoded demo alerts withisDemoData: true. When not in demo mode, shows install prompt and empty state.Add image preview button to feedback screenshot thumbnails #4199 - Add image preview button to feedback screenshot thumbnails: Added Eye/preview button to screenshot thumbnails in the feedback modal. Clicking opens a fullscreen image overlay with the screenshot at full resolution. Overlay is dismissible via Escape key, close button, or backdrop click. Reuses existing
Maximize2andEyeicons already imported.Deployment status mismatch: list card shows degraded/failed while drilldown shows healthy #4200 - Deployment status mismatch between list and drilldown: DeploymentDrillDown now reconciles
reason/messagewith live kubectl data after fetching. When the live fetch shows the deployment is healthy (readyReplicas === replicas), stale failurereason/messagefrom the click payload is cleared. When still unhealthy, reason is derived from the deployment's actual conditions.ArgoCD cards show Demo mode even when ArgoCD is deployed and running #4201 - ArgoCD cards show Demo mode even when ArgoCD is deployed: ArgoCD hooks (
useArgoCDApplications,useArgoCDHealth,useArgoCDSyncStatus) no longer force mock fallback when the API returns empty items or zero totals withisDemoData=false. Previously, an empty result from a running ArgoCD was treated as "no ArgoCD" and triggered demo data. Integration banner in all three ArgoCD cards is now conditional onisDemoData.🐛 Console shows cluster as reachable when API server is externally unreachable #4202 - Console shows cluster as reachable when API server is externally unreachable: Added external TCP probe (
probeAPIServer) in the Go backend'sGetClusterHealth. NewexternallyReachablefield onClusterHealthdistinguishes internal vs external reachability. The ClusterHealth card shows a yellow "ext. unreachable" warning badge when the cluster is internally healthy but the API server TCP probe fails.Files changed
pkg/k8s/client.go- External reachability TCP probe + new fieldweb/src/hooks/useArgoCD.ts- Fix aggressive mock fallbackweb/src/hooks/mcp/types.ts- Add externallyReachable to typesweb/src/hooks/mcp/shared.ts- Pass through externallyReachableweb/src/components/cards/ComplianceCards.tsx- FalcoAlerts demo gatingweb/src/components/cards/ArgoCDApplications.tsx- Conditional bannerweb/src/components/cards/ArgoCDHealth.tsx- Conditional bannerweb/src/components/cards/ArgoCDSyncStatus.tsx- Conditional bannerweb/src/components/cards/ClusterHealth.tsx- External unreachable badgeweb/src/components/drilldown/views/DeploymentDrillDown.tsx- Status reconciliationweb/src/components/feedback/FeatureRequestModal.tsx- Screenshot previewTest plan
Closes #4198, closes #4199, closes #4200, closes #4201, closes #4202