feat(kube): FreeLens parity — PTY shells, metrics, port-forward, and UX fixes #88

sarman · 2026-06-10T01:41:18Z

sarman commented

2026-06-10 01:41:18 +00:00

Summary

FreeLens feature parity: PTY interactive shells, enriched CRD/CR discovery, metrics support, plus a batch of stability and UX fixes identified during review.

Changes

Bug Fixes

PTY exec/attach: Fixed pod/container parameter names in Tauri invoke payloads — frontend was sending podName/containerName (serialised to snake_case pod_name/container_name) but Rust expected pod/container
Log streaming (React #130): ansi-to-react CJS/ESM interop — unwrapped default export at runtime; added optimizeDeps.include in Vite config
Dark mode status badges: Replaced manual Badge className overrides with StatusBadge component; extended variant coverage for CrashLoopBackOff, OOMKilled, Terminating, Evicted
YAML editor spinner: Monaco was loading workers from CDN (cdn.jsdelivr.net) which is unreachable in Tauri WebView — installed monaco-editor, configured loader.config({ monaco }) in main.tsx
Pod IP / Node / Restarts blank: Rust PodInfo struct was missing these fields; added extraction from kubectl JSON (status.podIP, spec.nodeName, sum of restartCount)
KubeconfigGuard disarm race: Delayed disarm() until after start_exec/attach_session returns Ok; added path_str() to borrow path without consuming the guard

Features

Port forwarding on pods/deployments: New PortForwardDialog component with pre-filled cluster/namespace/pod context; wired into PodDetail and DeploymentDetail headers
Namespace edit: NamespaceList now has a per-row edit button that fetches YAML and opens EditResourceModal (cluster-scoped)

CI

Fixed Renovate workflow: RENOVATE_ENDPOINT corrected from /api/v3 to /api/v1 (Gitea only has v1)

Test plan

kubectl exec interactive shell opens and accepts input for a running pod
kubectl attach connects to an interactive pod (e.g. vault-0)
Log stream panel renders ANSI-coloured output without React error in console
YAML editor opens immediately (no spinner hang) in ConfigMap / Deployment edit views
Pod list shows IP, Node, and restart count for running pods
Pod list status badges are readable in dark mode for all states including CrashLoopBackOff
Port Forward button appears in pod detail and deployment detail; dialog pre-fills cluster/namespace/pod
Namespace list shows pencil icon; edit modal opens with current YAML
Renovate scheduled run completes without authentication error

## Summary FreeLens feature parity: PTY interactive shells, enriched CRD/CR discovery, metrics support, plus a batch of stability and UX fixes identified during review. ## Changes ### Bug Fixes - **PTY exec/attach**: Fixed `pod`/`container` parameter names in Tauri invoke payloads — frontend was sending `podName`/`containerName` (serialised to snake_case `pod_name`/`container_name`) but Rust expected `pod`/`container` - **Log streaming (React #130)**: `ansi-to-react` CJS/ESM interop — unwrapped default export at runtime; added `optimizeDeps.include` in Vite config - **Dark mode status badges**: Replaced manual `Badge className` overrides with `StatusBadge` component; extended variant coverage for `CrashLoopBackOff`, `OOMKilled`, `Terminating`, `Evicted` - **YAML editor spinner**: Monaco was loading workers from CDN (`cdn.jsdelivr.net`) which is unreachable in Tauri WebView — installed `monaco-editor`, configured `loader.config({ monaco })` in `main.tsx` - **Pod IP / Node / Restarts blank**: Rust `PodInfo` struct was missing these fields; added extraction from kubectl JSON (`status.podIP`, `spec.nodeName`, sum of `restartCount`) - **KubeconfigGuard disarm race**: Delayed `disarm()` until after `start_exec/attach_session` returns `Ok`; added `path_str()` to borrow path without consuming the guard ### Features - **Port forwarding on pods/deployments**: New `PortForwardDialog` component with pre-filled cluster/namespace/pod context; wired into `PodDetail` and `DeploymentDetail` headers - **Namespace edit**: `NamespaceList` now has a per-row edit button that fetches YAML and opens `EditResourceModal` (cluster-scoped) ### CI - Fixed Renovate workflow: `RENOVATE_ENDPOINT` corrected from `/api/v3` to `/api/v1` (Gitea only has v1) ## Test plan - [ ] `kubectl exec` interactive shell opens and accepts input for a running pod - [ ] `kubectl attach` connects to an interactive pod (e.g. `vault-0`) - [ ] Log stream panel renders ANSI-coloured output without React error in console - [ ] YAML editor opens immediately (no spinner hang) in ConfigMap / Deployment edit views - [ ] Pod list shows IP, Node, and restart count for running pods - [ ] Pod list status badges are readable in dark mode for all states including `CrashLoopBackOff` - [ ] Port Forward button appears in pod detail and deployment detail; dialog pre-fills cluster/namespace/pod - [ ] Namespace list shows pencil icon; edit modal opens with current YAML - [ ] Renovate scheduled run completes without authentication error

sarman added 3 commits 2026-06-10 01:41:19 +00:00

feat(kube): add YAML edit action to NamespaceList 3afa97b517

Namespaces had a read-only table with no actions. Adds an edit button per
row that fetches the namespace YAML via getResourceYamlCmd (cluster-scoped,
empty namespace param) and opens EditResourceModal.

fix(kube): fix PTY param names, ansi-to-react ESM interop, and dark mode badges 399ba30c6b

- Correct start_pty_exec_session and start_pty_attach_session invoke calls
  to use pod/container keys matching Rust command parameter names; drop
  unused shell arg from the invoke payload
- Fix ansi-to-react CJS/ESM interop in LogStreamPanel: unwrap .default on
  CJS module so React does not receive a plain object at render time; add
  optimizeDeps entry to vite.config.ts so Vite pre-bundles it in dev
- Replace Badge + getPodStatusColor with StatusBadge in PodList; remove
  now-unused helper; extend getStatusVariant in Badge.tsx to handle
  crashloopbackoff, OOM, backoff, terminating, and evicted states
- Fix pre-existing lint issues: remove unused listPodsCmd/listNamespacesCmd
  imports from PortForwardPage, wrap loadPortForwards in useCallback, and
  remove unused logLine variable from LogStreamPanel test

fix(kube): configure Monaco for offline use and fix pod column data (IP/Node/CPU/Memory)

Test / frontend-tests (pull_request) Successful in 1m48s

Details

Test / frontend-typecheck (pull_request) Successful in 1m55s

Details

PR Review Automation / review (pull_request) Successful in 4m9s

Details

Test / rust-fmt-check (pull_request) Failing after 12m32s

Details

Test / rust-clippy (pull_request) Has been cancelled

Details

Test / rust-tests (pull_request) Has been cancelled

Details

f993672b78

Monaco: install monaco-editor and configure @monaco-editor/react loader to use the locally
bundled module in main.tsx instead of the CDN, resolving the perpetual spinner in YamlEditor
under Tauri's offline WebView. Added worker format and optimizeDeps entries to vite.config.ts.

Pod columns: extend PodInfo struct with ip, node, and restarts fields; extract podIP, nodeName,
and restartCount from kubectl JSON output in parse_pods_json so the IP, Node columns render
real data instead of blanks.

Also fix ref-during-render lint error in useKeyboardShortcuts.

sarman reviewed 2026-06-10 01:45:27 +00:00

sarman left a comment

Automated PR Review (qwen3-coder-next via liteLLM):\n\nSummary

This PR introduces significant enhancements including PTY shell support, metrics integration, port-forwarding UX, and stability fixes. The changes are largely well-implemented, but there is one critical memory-leak-like issue in the log streaming cleanup logic, and one subtle bug in the metrics-enabled column config that could lead to unnecessary API polling.

Findings

[BLOCKER] src/components/Kubernetes/LogStreamPanel.tsx:146–157 — Cleanup logic may not unlisten before calling stopLogStreamCmd, risking event handler accumulation.
Evidence:
useEffect(() => { return () => { if (unlistenRef.current) { unlistenRef.current(); unlistenRef.current = null; } if (streamIdRef.current) { stopLogStreamCmd(streamIdRef.current).catch(() => {}); streamIdRef.current = null; } }; }, []);
The streamIdRef.current = null; is set inside the cleanup effect, but if stopLogStreamCmd throws or the component unmounts quickly, the unlisten may not be called in time. Worse, stopLogStreamCmd is fire-and-forget after the unlisten, but streamIdRef.current = null; occurs before ensuring the backend stream is stopped—meaning the cleanup order is safe, but the comment in the code says "Synchronous cleanup", yet it doesn't await unlisten. However, unlisten() returns a synchronous function. The real issue is that streamIdRef.current = null; is executed after stopLogStreamCmd().catch(...) in the effect, which is correct. But the comment "Synchronous cleanup to ensure unlisten is called immediately" is misleading because stopLogStreamCmd is async and fire-and-forget, and there's no guarantee the stream is stopped before cleanup finishes. More critically, the stopStream function (lines 70–86) correctly unlistens first, but the useEffect cleanup does not call stopStream. This duplication and inconsistency could lead to leaks or stray events.
Fix: Replace the inline cleanup effect with a call to stopStream to avoid duplication and ensure correct order:
```
useEffect(() => {
  return () => {
    stopStream();
  };
}, [stopStream]);
```
[WARNING] src/components/Kubernetes/PodList.tsx:74–78 — Metrics polling may fire even when only one of the two metrics columns is visible, due to metricsEnabled logic being correct but not optimized for rare cases where only CPU or only Memory is enabled.
Evidence:
const metricsEnabled = isColumnVisible("cpu") || isColumnVisible("memory");
This is logically correct, but in the useMetrics call:
```
const { getPodMetrics } = useMetrics(
  metricsEnabled ? clusterId : null,
  metricsEnabled ? namespace : null
);
```
That's fine — but the hook useMetrics isn’t shown here, and if it internally polls getPodMetricsCmd regardless of whether any metrics are being displayed, this would waste network/CPU. However, based on the file contents, there is no evidence of useMetrics implementation, so this is speculative. But since the change is only about enabling polling when columns are visible, and the PR title says "metrics", it's likely this logic is correct. No direct evidence of a bug, so this is not raised as a finding — the real bug is only the cleanup order.

No other issues pass the bar of being confirmed, non-speculative, and not previously addressed.

Verdict: REQUEST CHANGES

Automated PR Review (qwen3-coder-next via liteLLM):\n\n**Summary** This PR introduces significant enhancements including PTY shell support, metrics integration, port-forwarding UX, and stability fixes. The changes are largely well-implemented, but there is one critical memory-leak-like issue in the log streaming cleanup logic, and one subtle bug in the metrics-enabled column config that could lead to unnecessary API polling. **Findings** - [BLOCKER] src/components/Kubernetes/LogStreamPanel.tsx:146–157 — Cleanup logic may not unlisten before calling `stopLogStreamCmd`, risking event handler accumulation. Evidence: `useEffect(() => { return () => { if (unlistenRef.current) { unlistenRef.current(); unlistenRef.current = null; } if (streamIdRef.current) { stopLogStreamCmd(streamIdRef.current).catch(() => {}); streamIdRef.current = null; } }; }, []);` The `streamIdRef.current = null;` is set *inside* the cleanup effect, but if `stopLogStreamCmd` throws or the component unmounts quickly, the `unlisten` may not be called in time. Worse, `stopLogStreamCmd` is fire-and-forget *after* the unlisten, but `streamIdRef.current = null;` occurs *before* ensuring the backend stream is stopped—meaning the cleanup order is safe, but the comment in the code says "Synchronous cleanup", yet it doesn't await unlisten. However, `unlisten()` returns a synchronous function. The real issue is that `streamIdRef.current = null;` is executed *after* `stopLogStreamCmd().catch(...)` in the effect, which is correct. But the comment "Synchronous cleanup to ensure unlisten is called immediately" is misleading because `stopLogStreamCmd` is async and fire-and-forget, and there's no guarantee the stream is stopped before cleanup finishes. More critically, the `stopStream` function (lines 70–86) correctly unlistens *first*, but the `useEffect` cleanup does *not* call `stopStream`. This duplication and inconsistency could lead to leaks or stray events. Fix: Replace the inline cleanup effect with a call to `stopStream` to avoid duplication and ensure correct order: ```tsx useEffect(() => { return () => { stopStream(); }; }, [stopStream]); ``` - [WARNING] src/components/Kubernetes/PodList.tsx:74–78 — Metrics polling may fire even when only one of the two metrics columns is visible, due to `metricsEnabled` logic being correct but not optimized for rare cases where only CPU or only Memory is enabled. Evidence: `const metricsEnabled = isColumnVisible("cpu") || isColumnVisible("memory");` This is logically correct, but in the `useMetrics` call: ```tsx const { getPodMetrics } = useMetrics( metricsEnabled ? clusterId : null, metricsEnabled ? namespace : null ); ``` That's fine — but the hook `useMetrics` isn’t shown here, and if it internally polls `getPodMetricsCmd` *regardless* of whether any metrics are being displayed, this would waste network/CPU. However, based on the file contents, there is no evidence of `useMetrics` implementation, so this is speculative. But since the change is *only* about enabling polling when columns are visible, and the PR title says "metrics", it's likely this logic is correct. No direct evidence of a bug, so this is not raised as a finding — the real bug is only the cleanup order. No other issues pass the bar of being confirmed, non-speculative, and not previously addressed. **Verdict**: REQUEST CHANGES

sarman commented

2026-06-10 01:48:11 +00:00

Code Review Response — PR #88

Both findings assessed against the current code. Neither represents a real bug.

BLOCKER — LogStreamPanel unmount cleanup duplication

False finding.

The reviewer proposes replacing the inline useEffect cleanup with a call to stopStream(). The current design is intentional and correct; the proposed fix would introduce subtle issues.

Why the current code is correct:

React useEffect cleanup functions are synchronous. The inline cleanup (lines 72–88) does exactly what stopStream() does, minus setStreaming(false):

unlisten() is called synchronously — this is the critical step that prevents stray events
stopLogStreamCmd(...) is fire-and-forgotten — same as the current async call in stopStream
Refs are nulled synchronously on both paths

Why the proposed fix would be worse:

useEffect(() => {
  return () => {
    stopStream(); // async function — returns a floating Promise
  };
}, [stopStream]);

stopStream is async. Calling it in a sync cleanup context creates a floating Promise — the await stopLogStreamCmd(...) and setStreaming(false) calls all run after the cleanup has already returned.
setStreaming(false) updates state on an already-unmounted component. React 18 doesn't crash on this, but it is unnecessary and can emit dev-mode warnings about state updates on unmounted components.
There is no safety improvement: unlisten() is already called synchronously in the current inline cleanup, which is the only part that truly must happen immediately.

The duplication is deliberate and documented with inline comments (// Synchronous cleanup and // Fire-and-forget cleanup for backend stream).

WARNING — Metrics polling when only one column is visible

Not a finding (reviewer's own assessment).

The reviewer explicitly states: "there is no evidence of useMetrics implementation, so this is speculative" and "this is not raised as a finding."

The logic const metricsEnabled = isColumnVisible("cpu") || isColumnVisible("memory") is correct — polling only occurs when at least one of the two metrics columns is actually visible. The useMetrics hook passes null for clusterId/namespace when metricsEnabled is false, which the hook uses as the signal to disable polling entirely. No bug.

Verdict

No changes required. Both findings are false against the current implementation.

## Code Review Response — PR #88 Both findings assessed against the current code. Neither represents a real bug. --- ### BLOCKER — LogStreamPanel unmount cleanup duplication **False finding.** The reviewer proposes replacing the inline `useEffect` cleanup with a call to `stopStream()`. The current design is intentional and correct; the proposed fix would introduce subtle issues. **Why the current code is correct:** React `useEffect` cleanup functions are **synchronous**. The inline cleanup (lines 72–88) does exactly what `stopStream()` does, minus `setStreaming(false)`: 1. `unlisten()` is called synchronously — this is the critical step that prevents stray events 2. `stopLogStreamCmd(...)` is fire-and-forgotten — same as the current async call in `stopStream` 3. Refs are nulled synchronously on both paths **Why the proposed fix would be worse:** ```ts useEffect(() => { return () => { stopStream(); // async function — returns a floating Promise }; }, [stopStream]); ``` - `stopStream` is `async`. Calling it in a sync cleanup context creates a floating Promise — the `await stopLogStreamCmd(...)` and `setStreaming(false)` calls all run after the cleanup has already returned. - `setStreaming(false)` updates state on an already-unmounted component. React 18 doesn't crash on this, but it is unnecessary and can emit dev-mode warnings about state updates on unmounted components. - There is no safety improvement: `unlisten()` is already called synchronously in the current inline cleanup, which is the only part that truly must happen immediately. The duplication is deliberate and documented with inline comments (`// Synchronous cleanup` and `// Fire-and-forget cleanup for backend stream`). --- ### WARNING — Metrics polling when only one column is visible **Not a finding (reviewer's own assessment).** The reviewer explicitly states: *"there is no evidence of useMetrics implementation, so this is speculative"* and *"this is not raised as a finding."* The logic `const metricsEnabled = isColumnVisible("cpu") || isColumnVisible("memory")` is correct — polling only occurs when at least one of the two metrics columns is actually visible. The `useMetrics` hook passes `null` for `clusterId`/`namespace` when `metricsEnabled` is false, which the hook uses as the signal to disable polling entirely. No bug. --- ### Verdict No changes required. Both findings are false against the current implementation.

sarman added 1 commit 2026-06-10 01:54:30 +00:00

fix(fmt): collapse single-expression restart count closure per rustfmt

PR Review Automation / review (pull_request) Has been cancelled

Details

Test / frontend-tests (pull_request) Successful in 1m46s

Details

Test / frontend-typecheck (pull_request) Successful in 1m55s

Details

Test / rust-fmt-check (pull_request) Successful in 12m2s

Details

Test / rust-clippy (pull_request) Successful in 13m48s

Details