feat: enhance log window to allow configuring options for viewing log… #417
Conversation
…s and support text copying. Signed-off-by: David Hettinger <[email protected]> Signed-off-by: Hettinger, David <[email protected]>
adameska
force-pushed
the
AllowGettingPreviousLogs
branch
from
27a4bc3 to
97f0afd
Compare
|
Thanks @adameska for this contribution, which is very valuable. To help the team review, test and discuss it, it would be preferable to split this PR into a few ones, to add independent enhancements separately. I can see these independent enhancements:
|
Thanks for the response! I can split this out into 3 separate PRs since i think splitting out all the options separately would be a bit of a merge nightmare unless done in many steps...
I will keep this PR open as the 1st option of adding the options object. |
- Added scroll functionality to view previous logs - Updated terminal color handling - Improved pod logs component Signed-off-by: adameska <[email protected]>
Signed-off-by: adameska <[email protected]>
Signed-off-by: adameska <[email protected]>
adameska
force-pushed
the
AllowGettingPreviousLogs
branch
from
4493cb6 to
a311bfd
Compare
Signed-off-by: adameska <[email protected]>
Signed-off-by: Hettinger, David <[email protected]>
Signed-off-by: David Hettinger <[email protected]>
Signed-off-by: David Hettinger <[email protected]>
|
I realized I hooked into the "editor" settings not "terminal" settings. Since updating to that it's now also honoring terminal line height as well as font-size (and scrollback but i can't seem to set that in the UI? EDIT: After updating i see it now, and it will be honored) |
Signed-off-by: Adameska <[email protected]>
Signed-off-by: Adameska <[email protected]>
|
Add a kebab menu for settings, can be further enhanced later if needed |
📝 WalkthroughWalkthroughAdds TerminalSettings channel/state/dispatcher and PodLogsOptions type; threads PodLogsOptions from channels through API, service, stream layer, and UI; extends PodLogs UI and TerminalWindow with configurable font/line/scrollback settings; minor .gitignore and webview dev-dep updates. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as PodLogs.svelte
participant Stream as StreamLayer (pod-logs.ts)
participant API as PodLogsApi
participant Service as PodLogsService
participant K8s as Kubernetes API
UI->>Stream: subscribe(pod, ns, container, cb, options?)
Stream->>API: streamPodLogs(pod, ns, container, options?)
API->>Service: startStream(pod, ns, container, options?)
Service->>K8s: request logs with options
loop streaming
K8s-->>Service: log chunk
Service-->>API: emit chunk
API-->>Stream: deliver chunk
Stream-->>UI: callback(chunk)
UI->>UI: render (color/timestamps/per-options)
end
sequenceDiagram
participant Extension as Extension Host
participant Dispatcher as TerminalSettingsDispatcher
participant Config as Config API
participant RPC as TERMINAL_SETTINGS
participant Webview as StateTerminalSettingsInfo
participant Terminal as TerminalWindow
Extension->>Dispatcher: instantiate dispatcher
Dispatcher->>Config: read terminal.integrated.* settings
Config-->>Dispatcher: return settings
Dispatcher->>RPC: publish TerminalSettings
Webview->>RPC: initChannel(TERMINAL_SETTINGS)
RPC-->>Webview: receive TerminalSettings
Webview->>Terminal: supply fontSize/lineHeight/lineCount props
Terminal->>Terminal: apply settings via FitAddon / re-fit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (1)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/webview/src/stream/pod-logs.ts (1)
48-60: Clean up the POD_LOGS listener ifstreamPodLogsfailsRight now the listener is registered with
rpcBrowser.onbeforestreamPodLogsis awaited. IfstreamPodLogsrejects (for example, due to missing current context or a transport error),subscriberejects, but the listener stays attached and you have noIDisposablehandle to remove it. On a retried subscribe, this can lead to duplicate callbacks and a slow-growing leak of listeners.You can guard against this by disposing the listener in the error path:
const disposable = this.rpcBrowser.on(POD_LOGS, chunk => { if (chunk.podName !== podName || chunk.namespace !== namespace || chunk.containerName !== containerName) { return; } callback(chunk); }); - await this.#podLogsApi.streamPodLogs(podName, namespace, containerName, options); + try { + await this.#podLogsApi.streamPodLogs(podName, namespace, containerName, options); + } catch (err) { + // ensure we don't leak a listener when the stream fails to start + disposable.dispose(); + throw err; + }The
disposeimplementation below remains unchanged and continues to callstopStreamPodLogswhen the subscription is torn down normally.
🧹 Nitpick comments (9)
packages/webview/src/stream/pod-logs.spec.ts (1)
58-61: Tests correctly cover new options parameter; consider also asserting stopStreamPodLogs behaviorThe expectations against
streamPodLogs(..., undefined)correctly validate that the new optional options argument is wired through when not provided. SinceStreamPodLogs.subscribenow callsstopStreamPodLogsin itsdisposepath, you might want to extend this test to assert thatpodLogsApiMock.stopStreamPodLogsis invoked whensubscribeResultX.dispose()is called, so the new lifecycle behavior is covered as well.packages/channels/src/interface/pod-logs-api.ts (1)
21-27: Avoid duplicatingPodLogsOptionstype definition
PodLogsOptionsis defined here as a type alias, and there is also an interface of the same name inpackages/channels/src/model/pod-logs-options.ts. Maintaining two separate definitions increases the risk of divergence if fields are added/changed later.Consider importing and reusing the model type here instead of redefining it:
-import type { PodLogsOptions } from '../model/pod-logs-options'; - -export type PodLogsOptions = { - stream?: boolean; - previous?: boolean; - tailLines?: number; - sinceSeconds?: number; - timestamps?: boolean; -}; +import type { PodLogsOptions } from '../model/pod-logs-options';This keeps a single canonical
PodLogsOptionsshape shared across model and interface layers.packages/extension/src/manager/contexts-states-dispatcher.ts (1)
113-121: Initial TERMINAL_SETTINGS dispatch is fine but may depend on init orderingCalling
this.dispatch(TERMINAL_SETTINGS).catch(console.error);ininit()will push terminal settings immediately to any subscribers that already exist. If subscribers are registered only afterinit()runs, this call will effectively be a no-op and the actual first dispatch will still be triggered via theonSubscribecallbacks below.That’s functionally correct, just be aware that the usefulness of this eager dispatch depends on when
StateTerminalSettingsInfo.init()(and thus subscription) happens relative toContextsStatesDispatcher.init(). If you intended a guaranteed initial push regardless of ordering, you might instead rely solely on theonSubscribepath, or explicitly document the expected init sequence.packages/extension/src/manager/pod-logs-api-impl.ts (1)
39-57: Options are correctly threaded through to the underlying PodLogsServiceExtending
streamPodLogsto acceptoptions?: PodLogsOptionsand passing those options only whencounter === 1preserves the existing reference-counted behavior while allowing the first subscriber to configure the stream. For the current UI (single logs view per pod/container), this is a reasonable trade-off.Be aware that if you ever support multiple concurrent consumers for the same
(podName, namespace, containerName)with different options, the first caller’s options will “win” for the shared stream; further callers’ options are ignored by design.packages/webview/src/component/pods/PodLogs.spec.ts (1)
45-65: Terminal settings mock setup is sound; consider asserting settings usage in the futureInitializing
terminalSettingsMockviastatesMocks.mock('stateTerminalSettingsInfoUI', terminalSettingsMock), setting default values, and stubbingsubscribeto return an unsubscribe function ensures the new state dependency doesn’t break existing tests.If you later expand coverage, you could also assert that
TerminalWindowis called with props reflecting these settings (font size, line height, scrollback), but that’s a nice-to-have rather than necessary for this PR.packages/webview/src/component/pods/PodLogs.svelte (2)
36-37: Sanitize numeric log options before using them
tailLinesandsinceSecondscome from number inputs and are passed directly intoPodLogsOptionsand aslineCountforTerminalWindow. When the fields are cleared or invalid, these can becomenull/NaN/empty, which may propagate unexpected values to the backend and toscrollback.Consider normalizing them to
number | undefinedbefore buildingoptionsand computinglineCount, e.g.:- let tailLines = $state<number | undefined>(lineCount); - let sinceSeconds = $state<number | undefined>(undefined); + let tailLines = $state<number | undefined>(lineCount); + let sinceSeconds = $state<number | undefined>(undefined); + function toNumberOrUndefined(value: unknown): number | undefined { + return typeof value === 'number' && !Number.isNaN(value) ? value : undefined; + } const options: PodLogsOptions = { - stream: isStreaming, - previous, - tailLines, - sinceSeconds, - timestamps, + stream: isStreaming, + previous, + tailLines: toNumberOrUndefined(tailLines), + sinceSeconds: toNumberOrUndefined(sinceSeconds), + timestamps, }; <TerminalWindow class="h-full" bind:terminal={logsTerminal} convertEol disableStdIn - fontSize={fontSize} - lineHeight={lineHeight} - lineCount={tailLines ?? lineCount} /> + fontSize={fontSize} + lineHeight={lineHeight} + lineCount={toNumberOrUndefined(tailLines) ?? lineCount} />This keeps the UI flexible while ensuring the stream options and terminal scrollback always see valid numbers.
Also applies to: 109-115, 188-196, 260-266
30-42: Make line height (and possibly scrollback) reactive to terminal settingsYou already react to terminal settings changes for
fontSize, butlineHeightand the scrollback-derived defaults stay fixed after the component is created. If users change terminal preferences at runtime, Pod logs will only partially pick up those changes.If you want full alignment with terminal preferences, consider:
- const lineCount = terminalSettingsState.data?.scrollback ?? 1000; + let lineCount = terminalSettingsState.data?.scrollback ?? 1000; ... - let lineHeight = $state(terminalSettingsState.data?.lineHeight ?? 1); + let lineHeight = $state(terminalSettingsState.data?.lineHeight ?? 1); ... -$effect(() => { - if (terminalSettingsState.data?.fontSize !== undefined) { - fontSize = terminalSettingsState.data.fontSize; - } -}); +$effect(() => { + const data = terminalSettingsState.data; + if (!data) { + return; + } + if (data.fontSize !== undefined) { + fontSize = data.fontSize; + } + if (data.lineHeight !== undefined) { + lineHeight = data.lineHeight; + } + if (data.scrollback !== undefined && tailLines === lineCount) { + // Only update default tail when user hasn't overridden it + lineCount = data.scrollback; + tailLines = data.scrollback; + } +});This keeps Pod logs in sync with global terminal settings without overriding user tweaks to the Tail field.
Also applies to: 49-55
packages/extension/src/dispatcher/_dispatcher-module.ts (2)
19-33: Consider organizing imports and bindings alphabetically.The imports are roughly alphabetical, but the dispatcher bindings below (lines 36-73) follow a different order. Keeping both sections in the same alphabetical order improves maintainability and makes it easier to locate specific dispatchers.
54-56: Remove inconsistent blank line.Line 56 introduces a blank line between
TerminalSettingsDispatcherandUpdateResourceDispatcherbindings, which is inconsistent with the spacing used for other dispatcher pairs in this file.Apply this diff to remove the extra blank line:
options.bind<TerminalSettingsDispatcher>(TerminalSettingsDispatcher).toSelf().inSingletonScope(); options.bind(DispatcherObject).toService(TerminalSettingsDispatcher); - options.bind<UpdateResourceDispatcher>(UpdateResourceDispatcher).toSelf().inSingletonScope(); options.bind(DispatcherObject).toService(UpdateResourceDispatcher);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
.gitignore(1 hunks)packages/channels/src/channels.ts(3 hunks)packages/channels/src/interface/pod-logs-api.ts(1 hunks)packages/channels/src/model/index.ts(1 hunks)packages/channels/src/model/pod-logs-options.ts(1 hunks)packages/channels/src/model/terminal-settings.ts(1 hunks)packages/extension/src/dispatcher/_dispatcher-module.ts(2 hunks)packages/extension/src/dispatcher/terminal-settings-dispatcher.ts(1 hunks)packages/extension/src/manager/contexts-states-dispatcher.ts(2 hunks)packages/extension/src/manager/pod-logs-api-impl.ts(3 hunks)packages/extension/src/pod-logs/pod-logs-service.ts(3 hunks)packages/webview/package.json(1 hunks)packages/webview/src/component/pods/PodLogs.spec.ts(2 hunks)packages/webview/src/component/pods/PodLogs.svelte(4 hunks)packages/webview/src/component/pods/PodTerminal.svelte(1 hunks)packages/webview/src/component/terminal/TerminalWindow.svelte(5 hunks)packages/webview/src/component/terminal/TerminalWindows.spec.ts(2 hunks)packages/webview/src/state/state-module.ts(2 hunks)packages/webview/src/state/states.ts(2 hunks)packages/webview/src/state/terminal-settings.svelte.ts(1 hunks)packages/webview/src/stream/pod-logs.spec.ts(2 hunks)packages/webview/src/stream/pod-logs.ts(2 hunks)packages/webview/src/stream/util/stream-object.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
packages/channels/src/interface/pod-logs-api.ts (1)
packages/channels/src/model/pod-logs-options.ts (1)
PodLogsOptions(19-25)
packages/webview/src/state/terminal-settings.svelte.ts (4)
packages/extension/src/dispatcher/terminal-settings-dispatcher.ts (1)
injectable(25-46)packages/webview/src/state/states.ts (1)
injectable(34-126)packages/channels/src/model/terminal-settings.ts (1)
TerminalSettings(19-23)packages/channels/src/channels.ts (1)
TERMINAL_SETTINGS(73-73)
packages/extension/src/pod-logs/pod-logs-service.ts (2)
packages/channels/src/interface/pod-logs-api.ts (1)
PodLogsOptions(21-27)packages/channels/src/model/pod-logs-options.ts (1)
PodLogsOptions(19-25)
packages/channels/src/model/pod-logs-options.ts (1)
packages/channels/src/interface/pod-logs-api.ts (1)
PodLogsOptions(21-27)
packages/webview/src/stream/pod-logs.ts (5)
packages/channels/src/interface/pod-logs-api.ts (1)
PodLogsOptions(21-27)packages/channels/src/model/pod-logs-options.ts (1)
PodLogsOptions(19-25)packages/channels/src/index.ts (1)
IDisposable(21-21)packages/channels/src/types/disposable.ts (2)
IDisposable(18-18)IDisposable(20-22)packages/channels/src/channels.ts (1)
POD_LOGS(68-68)
packages/channels/src/channels.ts (1)
packages/channels/src/model/terminal-settings.ts (1)
TerminalSettings(19-23)
packages/extension/src/manager/pod-logs-api-impl.ts (2)
packages/channels/src/interface/pod-logs-api.ts (1)
PodLogsOptions(21-27)packages/channels/src/model/pod-logs-options.ts (1)
PodLogsOptions(19-25)
packages/extension/src/dispatcher/_dispatcher-module.ts (1)
packages/extension/src/dispatcher/util/dispatcher-object.ts (2)
DispatcherObject(22-22)DispatcherObject(23-26)
packages/webview/src/state/state-module.ts (1)
packages/webview/src/state/util/state-object.svelte.ts (2)
StateObject(23-23)StateObject(24-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build extension OCI image
🔇 Additional comments (24)
.gitignore (1)
10-10: Good addition to ignore Visual Studio metadata.Adding
/.vsto gitignore is standard practice, as this directory contains internal Visual Studio metadata and cache that shouldn't be committed. This complements the existing.ideapattern for JetBrains IDEs.packages/channels/src/model/terminal-settings.ts (1)
19-23: LGTM!The TerminalSettings interface is well-defined with appropriate properties for terminal configuration. The numeric types are suitable for fontSize, lineHeight, and scrollback settings.
packages/channels/src/channels.ts (2)
19-24: LGTM!The import reorganization improves code organization, and the TerminalSettings type import properly supports the new RPC channel.
Also applies to: 42-42
72-73: LGTM!The TERMINAL_SETTINGS channel follows the established pattern for RPC channels in this file and uses a clear, descriptive key.
packages/webview/src/component/pods/PodTerminal.svelte (1)
13-16: LGTM!Adding
containerName: stringto the Props interface correctly documents the component's required prop, which was already being used throughout the component's implementation.packages/channels/src/model/index.ts (1)
32-32: LGTM!The new exports for
pod-logs-optionsandterminal-settingsfollow the established pattern for model re-exports and properly expose the new types.Also applies to: 41-41
packages/webview/src/stream/util/stream-object.ts (1)
19-19: LGTM!The addition of the optional
podLogsOptionsparameter to thesubscribemethod maintains backward compatibility while enabling the new log viewing options feature.Also applies to: 28-28
packages/webview/src/component/terminal/TerminalWindows.spec.ts (2)
36-61: LGTM!The enhanced terminal mock setup provides comprehensive test coverage infrastructure with proper spies and mock implementations for Terminal methods, improving test isolation and assertion capabilities.
72-81: LGTM!The
createTerminalMockfunction now includes theoptionsobject and mock methods, creating a more complete Terminal mock for testing.packages/webview/package.json (1)
52-52: Verify the intentional downgrade of jsdom.The jsdom version is being downgraded from ^27.1.0 (on main) to ^27.0.0 on this PR branch. Version downgrades are unusual and may indicate an incompatibility issue, regression, or unintended change.
The 27.1.0 release notes (2025-10-31) document improvements to CSS parsing via
@acemir/cssom, selector cache invalidation fixes, and an engines field update—no regressions or breaking changes are mentioned. Additionally, packages/webview has no engines constraint that would conflict with 27.1.0.Please confirm this downgrade is intentional and, if so, document the reason (e.g., discovered incompatibility, breaking change, or dependency conflict not evident from release notes).
packages/channels/src/interface/pod-logs-api.ts (1)
29-31: API surface change forstreamPodLogslooks consistentThe updated signature
streamPodLogs(podName, namespace, containerName, options?: PodLogsOptions)matches how the options object is used in the extension and webview layers and keeps the old call sites valid by makingoptionsoptional. No issues from an API compatibility standpoint.packages/extension/src/manager/contexts-states-dispatcher.ts (1)
19-37: New TERMINAL_SETTINGS channel import usage is coherentBringing in
TERMINAL_SETTINGS,KUBERNETES_PROVIDERS, andRpcChannelhere keeps this dispatcher aligned with the broader channels set and typeddispatchsignature; nothing stands out as problematic.packages/webview/src/stream/pod-logs.ts (1)
41-47: Options propagation insubscribelooks correctAdding
options?: PodLogsOptionsand threading it through to the stream call makes the client-side API more flexible while keeping the previous call sites valid. The callback filtering bypodName,namespace, andcontainerNameis unchanged and still appropriate.packages/webview/src/state/terminal-settings.svelte.ts (1)
19-40: Terminal settings state wiring looks consistent with existing patterns
StateTerminalSettingsInfocleanly follows the existingAbsStateObjectImpl/StateObjectpattern and correctly binds to theTERMINAL_SETTINGSchannel ininit(). This should integrate smoothly with the state module and the new dispatcher.packages/extension/src/pod-logs/pod-logs-service.ts (2)
23-24: PodLogsOptions integration into PodLogsService is straightforwardImporting
PodLogsOptionsalongsidePOD_LOGSkeeps the service signature aligned with the channels API; no issues here.
36-66: Option mapping to Kubernetes Log API is correct — verified against @kubernetes/client-node v1.4.0 LogOptions interface. All mapped options (follow,previous,tailLines,sinceSeconds,timestamps) exist with matching types and semantics. The implementation correctly defaultsfollowtotruewhen omitted and preserves existing behavior whenoptionsis undefined.packages/webview/src/component/pods/PodLogs.spec.ts (2)
19-37: Test harness updates correctly integrate terminal settings state and streamsBringing in
TerminalSettings,TerminalWindow, and theRemoteMocks/StatesMocks/StreamsMockshelpers makes the PodLogs tests consistent with the new state and streaming architecture. The use ofFakeStateObject/FakeStreamObjectkeeps the tests focused on PodLogs behavior rather than wiring details.
95-123: Log rendering behavior tests remain valid with the new plumbingThe tests that assert clearing the terminal, writing received data (including the
\rsuffix), and toggling theEmptyScreenvisibility still accurately capture the expected behavior of PodLogs under the updated streaming/state setup. No issues here.packages/webview/src/state/states.ts (1)
21-32: Terminal settings state wiring matches existing patternsThe added
StateTerminalSettingsInfoimport, injection, and getter follow the same DI pattern as the otherState*Infoclasses. This should integrate cleanly with the rest of the state system.Also applies to: 106-111
packages/extension/src/dispatcher/terminal-settings-dispatcher.ts (1)
1-46: TerminalSettingsDispatcher implementation looks correctChannel wiring, configuration access, and default values are consistent with the existing dispatcher pattern.
getData()returns a well-formedTerminalSettingsobject; no issues spotted.packages/webview/src/component/pods/PodLogs.svelte (1)
63-69: Streaming options wiring and resource cleanup look solid
loadLogscorrectly clears the terminal, resets thenoLogsflag, disposes previous subscriptions, and re-subscribes with the newPodLogsOptions. The multi-container formatting (with optional colorful container names), the terminal settings subscription, and theonDestroycleanup for bothunsubscribersanddisposablesare all consistent and robust. The header controls and conditionalEmptyScreen/TerminalWindowvisibility nicely match the new behaviors.Also applies to: 85-107, 109-116, 117-140, 142-167, 248-259
packages/webview/src/state/state-module.ts (1)
34-37: Terminal settings state registration is consistent with other statesThe
StateTerminalSettingsInfobindings mirror the existing pattern (self singleton +StateObject+IDisposable). This should make terminal settings available everywhere viaStateswithout surprising behavior.Also applies to: 57-60
packages/webview/src/component/terminal/TerminalWindow.svelte (1)
21-23: Dynamic terminal sizing and scrollback integration look goodExposing
fontSize,lineHeight, andlineCountas props, wiringscrollbackat construction, and using a sharedFitAddonwith a reactive effect to apply updates is a clean way to let callers control terminal appearance. The guarded$effectand unchanged cleanup logic avoid leaks or race conditions, and theh-full w-fullcontainer ensures the terminal occupies the intended area.Also applies to: 34-36, 41-41, 76-79, 137-151, 170-170
packages/extension/src/dispatcher/_dispatcher-module.ts (1)
45-64: Verify that all new dispatchers are in scope for this PR.This PR adds several dispatchers beyond
TerminalSettingsDispatcher:
ResourcesCountDispatcher(lines 45-46)UpdateResourceDispatcher(lines 57-58)ResourceDetailsDispatcher(lines 60-61)ResourceEventsDispatcher(lines 63-64)The PR description and your comments focus on "allowing logging options to view previous/subset of logs." While
TerminalSettingsDispatcherclearly aligns with terminal configuration for log viewing, the resource-related dispatchers appear broader in scope.Confirm these are essential infrastructure for the logging feature rather than separate enhancements that should be split into a different PR.
Signed-off-by: Adameska <[email protected]>
Oh 👍 It is looking great. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/webview/src/component/pods/PodLogs.svelte (3)
30-30: MakelineCountreactive to reflect settings updates.
lineCountis initialized once fromterminalSettingsState.data?.scrollback ?? 1000but never updates. If terminal settings load after component initialization, the fallback value (1000) persists even though the actual scrollback setting is now available. This affects the comparison at line 61 and the fallback at line 287.Consider making
lineCountreactive:-const lineCount = terminalSettingsState.data?.scrollback ?? 1000; +const lineCount = $derived(terminalSettingsState.data?.scrollback ?? 1000);This ensures
lineCountalways reflects the current terminal settings.
120-126: Consider auto-reloading logs when options change.Currently, users must manually click the "Restart Stream" or "Retrieve Logs" button after changing options like
isStreaming,previous,tailLines, etc. Depending on UX requirements, you might want to add an effect that automatically callsloadLogs()when these options change, similar to how many log viewers work.Example implementation:
$effect(() => { // Watch option changes and reload isStreaming; previous; tailLines; sinceSeconds; timestamps; loadLogs().catch(console.error); });Note: This would reload on every option change, which may or may not be desirable. The current manual approach gives users more control.
269-273: Consider using{#if}block instead ofhiddenattribute for EmptyScreen.The
EmptyScreencomponent useshidden={noLogs === false}to control visibility, keeping it in the DOM when hidden. While this may be intentional for performance, using a conditional{#if noLogs}block would fully remove the component from the DOM when not needed, potentially improving memory usage and avoiding any hidden element side effects.- <EmptyScreen - icon={NoLogIcon} - title="No Log" - message="Log output of Pod {object.metadata?.name}" - hidden={noLogs === false} /> + {#if noLogs} + <EmptyScreen + icon={NoLogIcon} + title="No Log" + message="Log output of Pod {object.metadata?.name}" /> + {/if}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/channels/src/interface/pod-logs-api.ts(1 hunks)packages/webview/src/component/pods/PodLogs.svelte(3 hunks)packages/webview/src/stream/pod-logs.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/channels/src/interface/pod-logs-api.ts
- packages/webview/src/stream/pod-logs.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build / ubuntu-24.04
- GitHub Check: Build / macos-14
- GitHub Check: Build / windows-2022
- GitHub Check: Build extension OCI image
🔇 Additional comments (2)
packages/webview/src/component/pods/PodLogs.svelte (2)
61-64: Verify the tailLines update logic after addressing lineCount reactivity.This conditional update relies on comparing
tailLines === lineCount, which is fragile iflineCountremains a non-reactive constant (see line 30 comment). OncelineCountbecomes reactive, verify this logic still correctly detects whether the user has manually overriddentailLines.
237-253: I found the Podman Desktop core defaults (fontSize: 10, lineHeight: 1), but need to verify if there are explicit min/max constraints defined elsewhere.Let me search for any limits or validation constraints in the Podman Desktop codebase itself:
Let me search the codebase for any configuration schema or validation that might specify these limits:
The font size and line height input constraints (8-24 and 1-5) are reasonable UI safeguards and don't require alignment with Podman Desktop core settings.
Podman Desktop retrieves terminal settings from VS Code configuration (
terminal.integrated.fontSizedefaults to 10,terminal.integrated.lineHeightdefaults to 1), but does not enforce schema-level min/max bounds. The webview constraints in PodLogs.svelte are component-level UI safeguards. The limits fall within typical terminal ranges and encompass Podman Desktop's defaults plus a reasonable adjustment range for this specific component. No conflict or alignment issue exists.
Signed-off-by: Adameska <[email protected]>
| await this.dispatch(KUBERNETES_PROVIDERS); | ||
| }); | ||
|
|
||
| this.dispatch(TERMINAL_SETTINGS).catch(console.error); |
There was a problem hiding this comment.
I went through the effort of subscribing to setting changes but in the current state I don't think it's providing any value since when you go to the settings page it seems to close your pod page and you have to navigate back to it. I left this here in case that changes in the future (you can open and close settings without losing your state on the pod page).
| // Save colorfulOutput to localStorage whenever it changes | ||
| $effect(() => { | ||
| localStorage.setItem(colorfulOutputCacheKey, String(colorfulOutput)); |
There was a problem hiding this comment.
I'm not super familiar with this repository so if there's a more preferred way to store off a user preference specific to this page let me know. This seems to work fine as is for now.
…ng to fix that and it's mostly fixed now but randomly Signed-off-by: Adameska <[email protected]>
| if (noLogs) { | ||
| noLogs = false; | ||
| } | ||
| logsTerminal?.write(data + '\r', triggerResize); |
There was a problem hiding this comment.
These logs have been a little wonky, i've narrowed down some of the weirdness to calling resize on each chunk, now it will get called AFTER the buffer is done being written to and if we're loading a fresh log with lots of lines it won't call it until all lines have been rendered which made for a much better experience. It's still sometimes missing data which i thought maybe would be due to parallel calls to write but making those synchronous in order didn't seem to help so i removed that change...
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/webview/src/component/pods/PodLogs.svelte (1)
31-40: LocalStorage-basedcolorfulOutputpersistence is pragmatic but deviates from central preferencesPersisting
colorfulOutputvialocalStorageis simple and works, but it’s a one-off mechanism compared to the centralized terminal settings state you’re already using for font size, line height, and scrollback. If you want stronger consistency with the rest of Podman Desktop’s terminal preferences, consider flowing this through the same terminal settings channel instead of a dedicated localStorage key.Also applies to: 287-290
🧹 Nitpick comments (2)
packages/extension/src/manager/contexts-states-dispatcher.spec.ts (1)
112-113: Consider clarifying the comment for better readability.The comment is slightly awkward. Consider rephrasing for clarity:
-// Number of times TERMINAL_SETTINGS is dispatched when init() is called twice (once per init) +// Expected TERMINAL_SETTINGS dispatch count after two init() calls (one dispatch per init() call) const terminalSettingsCount = 2;packages/extension/src/manager/pod-logs-api-impl.ts (1)
39-69: Options threading is good; clarify multi-subscriber semantics with the counterExtending
streamPodLogsto takeoptions?: PodLogsOptionsand passing them straight through toPodLogsService.startStreamis clean, and the unconditionalstopStream()before starting ensures each call uses fresh options.However, the
counterstill tracks how many timesstreamPodLogshas been called vsstopStreamPodLogs, while the underlying stream is always stopped and restarted on everystreamPodLogscall:
- Multiple concurrent callers for the same
(podName, namespace, containerName)will all share a single underlyingPodLogsServiceinstance.- The options used for that shared stream will always be those from the latest
streamPodLogscall, regardless of what earlier callers requested.If the design guarantees a single logical caller per key (which is likely for a single PodLogs UI), this is fine. If not, you might want to document this behavior or consider per-caller isolation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/extension/src/manager/contexts-states-dispatcher.spec.ts(6 hunks)packages/extension/src/manager/pod-logs-api-impl.ts(2 hunks)packages/extension/src/pod-logs/pod-logs-service.ts(2 hunks)packages/webview/package.json(1 hunks)packages/webview/src/component/pods/PodLogs.svelte(2 hunks)packages/webview/src/component/terminal/TerminalWindows.spec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/webview/package.json
- packages/webview/src/component/terminal/TerminalWindows.spec.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/extension/src/manager/contexts-states-dispatcher.spec.ts (4)
packages/extension/src/manager/context-health-checker.ts (1)
ContextHealthState(26-32)packages/channels/src/channels.ts (4)
CONTEXTS_HEALTHS(55-55)CONTEXTS_PERMISSIONS(56-56)AVAILABLE_CONTEXTS(59-59)ENDPOINTS(63-63)packages/extension/src/manager/context-permissions-checker.ts (1)
ContextPermissionResult(55-58)packages/extension/src/manager/contexts-dispatcher.ts (1)
DispatcherEvent(25-28)
packages/extension/src/manager/pod-logs-api-impl.ts (1)
packages/channels/src/model/pod-logs-options.ts (1)
PodLogsOptions(19-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build extension OCI image
🔇 Additional comments (8)
packages/extension/src/manager/contexts-states-dispatcher.spec.ts (1)
115-256: Well-structured test updates to accommodate TERMINAL_SETTINGS dispatches.The test cases have been consistently updated to account for the new TERMINAL_SETTINGS dispatch behavior on each
init()call. The pattern is clear:
- First
init()dispatches TERMINAL_SETTINGS once- Second
init()dispatches TERMINAL_SETTINGS again, plus any event-triggered dispatches- The
terminalSettingsCountconstant improves maintainabilityAll arithmetic and expectations are correct across the test suite.
packages/webview/src/component/pods/PodLogs.svelte (5)
30-41: Terminal settings integration looks consistent and reactive
fontSize,lineHeight, andlineCountare driven fromterminalSettingsStateand wired through to<TerminalWindow>as props. The$effectkeeps local runes in sync with global settings while preserving user overrides fortailLines(tailLines === lineCountguard), which is a nice touch.Behaviorally, this means changing global terminal settings updates the rendering without unnecessarily reloading logs, while per-view overrides still work.
Also applies to: 56-72, 320-322
79-101: Stream lifecycle and disposal logic is sound
loadLogsfirst disposes existing subscriptions, clears the terminal, resetsnoLogs, then establishes new subscriptions with the currentPodLogsOptions.disposablesare also cleaned up inonDestroy, so we shouldn’t leak RPC streams when navigating away.Given that logs are per-container, the per-container
subscribeplus shareddisposablesarray is a reasonable approach.Also applies to: 145-172
231-249: Tail/since “unsynced changes” UX is clear and non-invasiveUsing
hasUnsyncedChangesto flag whentailLines/sinceSecondsdiffer from loaded values and surfacing that via the info icon next to the “Restart Stream / Retrieve Logs” button makes the “edit then explicitly reload” flow understandable without surprising auto-restarts.The explicit
loadLogscall on the button click ties this together nicely.Also applies to: 295-303
174-200: Settings menu lifecycle and click-outside handling look correctSubscribing to
terminalSettingsStateon mount, managingunsubscribers, and registering aclicklistener to close the settings menu when clicking outsidesettingsMenuRefare all cleaned up properly (viaonMount’s return andonDestroy). Thebind:this={settingsMenuRef}+ containment check is a straightforward and robust pattern here.Also applies to: 256-295
34-44: ****The claim about radio inputs producing string values is incorrect. In Svelte 5,
bind:group={isStreaming}withvalue={true}andvalue={false}(using curly braces) correctly produces boolean values, not strings. Svelte's bind:group uses the input's value expression—so you can have non-string values (numbers, booleans) by using value={…} The syntaxvalue={true}is a JavaScript expression, not an HTML string attribute.Additionally, PodLogsOptions.stream is typed as
stream?: boolean, and the button text logic ({isStreaming ? 'Restart Stream' : 'Retrieve Logs'}) confirms thatisStreamingis correctly treated as a boolean throughout the component. No changes are needed.Likely an incorrect or invalid review comment.
packages/extension/src/manager/pod-logs-api-impl.ts (1)
71-80: Retained reference counting for stop is still coherent
stopStreamPodLogs’s decrement-and-stop-when-zero logic is still coherent with the newstreamPodLogsbehavior: each successful subscribe should still be matched with a stop, and only when the last one callsstopStreamPodLogsdo you actually stop and remove the instance.As long as the webview side continues to pair every
streamPodLogscall with exactly onestopStreamPodLogs, this remains safe.packages/extension/src/pod-logs/pod-logs-service.ts (1)
36-81: Stream lifecycle and options mapping look correctThe
startStreamimplementation:
- Aborts and destroys any existing stream before starting a new one.
- Creates a fresh
PassThroughper call and wires it to emitPOD_LOGSchunks viarpcExtension.fire.- Maps
PodLogsOptionsinto the kubernetes client options with sensible defaults:follow: options?.stream ?? true, previous: options?.previous, tailLines: options?.tailLines, sinceSeconds: options?.sinceSeconds, timestamps: options?.timestamps,and
stopStreamcorrectly aborts and destroys current resources and clears the private fields.As long as callers pass a real boolean for
options.stream(see comment inPodLogs.svelte), this gives the expected “stream vs retrieve once” behavior.
Signed-off-by: Adameska <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
packages/webview/src/component/pods/PodLogs.svelte(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build extension OCI image
- GitHub Check: Build / windows-2022
- GitHub Check: Build / macos-14
- GitHub Check: Build / ubuntu-24.04
🔇 Additional comments (6)
packages/webview/src/component/pods/PodLogs.svelte (6)
52-72: Effects structure is well-designed.The effects correctly sync localStorage for
colorfulOutputand respond to terminal settings changes. Once thelineCountreactivity issue (line 30) is addressed, the conditional update logic on lines 68-71 will work as intended.
86-95: Debounced resize improves performance.The debounced resize handler addresses the performance issues mentioned in the past review comments. The 50ms delay strikes a good balance between responsiveness and avoiding excessive resize events during bulk log loading.
97-172:loadLogsorchestration is well-structured.The function correctly:
- Disposes previous subscriptions before reloading to prevent memory leaks
- Clears terminal state and syncs loaded values
- Propagates reactive options (
isStreaming,previous,tailLines,sinceSeconds,timestamps) intoPodLogsOptions- Handles multi-container scenarios with per-container colorization
- Uses the debounced resize callback to optimize bulk rendering
177-200: Lifecycle management is correct.The component properly:
- Subscribes to terminal settings on mount and unsubscribes on destroy
- Loads logs initially and sets up the click-outside handler with cleanup
- Disposes of all subscriptions and event listeners to prevent memory leaks
203-304: Header controls and settings menu provide excellent UX.The design thoughtfully differentiates between settings that reload immediately (mode, previous, timestamps) and those requiring manual action (tail, since). The
hasUnsyncedChangesindicator with tooltip guides users effectively. The kebab menu cleanly houses formatting preferences without cluttering the main controls.
310-323: Terminal rendering is well-implemented.The conditional visibility using
class:invisibleandclass:h-0whennoLogsis true provides a clean empty state. TheTerminalWindowintegration correctly passesfontSize,lineHeight, andlineCountprops. Note that line 322's fallback will work correctly once thelineCountreactivity issue (line 30) is resolved.
Signed-off-by: Adameska <[email protected]>
Signed-off-by: Adameska <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/webview/src/component/pods/PodLogs.svelte (2)
90-103: Clear pending timeout on component destruction to prevent post-unmount execution.The
resizeTimeoutmay fire after the component unmounts if destruction occurs within the 50ms window.Add cleanup in
onDestroy:onDestroy(() => { + if (resizeTimeout) { + clearTimeout(resizeTimeout); + } unsubscribers.forEach(unsubscriber => unsubscriber()); disposables.forEach(disposable => disposable.dispose()); disposables = []; });
158-176: Consider handling individual container subscription failures gracefully.Using
Promise.allmeans if any single container's subscription fails, all subscriptions are rejected. For pods with multiple containers, you may want partial success.Consider using
Promise.allSettledto handle individual failures:- const subscriptions = await Promise.all(subscriptionPromises); - disposables.push(...subscriptions); + const results = await Promise.allSettled(subscriptionPromises); + for (const result of results) { + if (result.status === 'fulfilled') { + disposables.push(result.value); + } else { + console.error('Failed to subscribe to container logs:', result.reason); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/webview/package.json(3 hunks)packages/webview/src/component/pods/PodLogs.svelte(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/webview/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build extension OCI image
🔇 Additional comments (4)
packages/webview/src/component/pods/PodLogs.svelte (4)
30-43: LGTM - State initialization with effect-based reactivity.The use of
$stateforlineCountcombined with the$effecton lines 69-74 properly addresses the reactivity concern for terminal settings changes. This is a valid alternative to$derived.
57-76: LGTM - Effect properly syncs terminal settings with user-override protection.The conditional check on line 70 correctly preserves user customizations when syncing scrollback from terminal settings.
182-198: LGTM - Lifecycle hooks with proper cleanup.The
onMountcorrectly returns a cleanup function for the click-outside event listener, andonDestroyproperly disposes subscriptions.
319-326: LGTM - TerminalWindow integration with configurable settings.The terminal correctly receives reactive props for font size, line height, and line count with appropriate fallback logic.
| object.metadata?.name ?? '', | ||
| object.metadata?.namespace ?? '', | ||
| containerName, |
There was a problem hiding this comment.
🧩 Analysis chain
Empty string fallbacks for metadata may cause API issues.
If object.metadata?.name or object.metadata?.namespace is undefined, passing empty strings to the subscription API may result in invalid API calls or unclear error messages.
Verify how the downstream API handles empty pod name or namespace:
🏁 Script executed:
#!/bin/bash
# Check how streamPodLogs handles empty name/namespace
rg -n -A10 "streamPodLogs" --type ts | head -50Length of output: 4218
I need to examine the PodLogsApi implementation to understand how it handles empty strings. Let me search for the API definition and implementation.
🏁 Script executed:
#!/bin/bash
# Find PodLogsApi definition
rg -n "PodLogsApi" --type ts -B2 -A5 | head -80Length of output: 6332
🏁 Script executed:
#!/bin/bash
# Look for the actual API implementation/interface
fd -e ts -e js | xargs grep -l "streamPodLogs" | head -10Length of output: 275
🏁 Script executed:
#!/bin/bash
# Check the PodLogs.svelte file around lines 160-162 for context
sed -n '150,170p' packages/webview/src/component/pods/PodLogs.svelteLength of output: 606
🏁 Script executed:
#!/bin/bash
# Check if there's validation before calling subscribe
rg -n "subscribe" packages/webview/src/component/pods/PodLogs.svelte -B5 -A5 | head -60Length of output: 1911
🏁 Script executed:
#!/bin/bash
# Look for backend API implementation (might be in a backend or api folder)
find . -type f \( -name "*.ts" -o -name "*.py" -o -name "*.go" \) | xargs grep -l "streamPodLogs" | grep -v node_modules | grep -v specLength of output: 186
Let me examine the API interface definition and implementation to understand how empty strings are handled.
🏁 Script executed:
#!/bin/bash
# Check the PodLogsApi interface definition
cat packages/channels/src/interface/pod-logs-api.tsLength of output: 1141
🏁 Script executed:
#!/bin/bash
# Check the PodLogsApiImpl implementation, particularly streamPodLogs method
cat packages/extension/src/manager/pod-logs-api-impl.tsLength of output: 3142
🏁 Script executed:
#!/bin/bash
# Find and check PodLogsService
fd -name "pod-logs-service*" --type f | head -5Length of output: 230
🏁 Script executed:
#!/bin/bash
# Look for any validation of pod name or namespace in the codebase
rg -n "podName|namespace" packages/extension/src/manager/pod-logs-api-impl.ts -B3 -A3Length of output: 1282
🏁 Script executed:
#!/bin/bash
# Check the pod-logs service implementation
find . -type f -name "*pod-logs*service*" | grep -v node_modules | grep -v specLength of output: 53
Let me check the PodLogsService implementation to see if there's validation:
🏁 Script executed:
#!/bin/bash
# Check PodLogsService implementation
cat packages/extension/src/pod-logs/pod-logs-service.tsLength of output: 2814
🏁 Script executed:
#!/bin/bash
# Check if there's any validation of empty values before API calls
rg -n "startStream|podName|namespace" packages/extension/src/pod-logs/pod-logs-service.ts -B2 -A2 | head -100Length of output: 518
Let me check test coverage and the V1Pod type definition:
🏁 Script executed:
#!/bin/bash
# Check tests for empty pod name/namespace scenarios
rg -n "empty|''|\"\"|undefined" packages/webview/src/component/pods/PodLogs.spec.ts -B2 -A2 | head -60Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any validation or guards in PodLogs.svelte around the subscribe call
sed -n '140,180p' packages/webview/src/component/pods/PodLogs.svelteLength of output: 1325
🏁 Script executed:
#!/bin/bash
# Check if there's any error handling after the subscribe call
sed -n '175,195p' packages/webview/src/component/pods/PodLogs.svelteLength of output: 587
🏁 Script executed:
#!/bin/bash
# Look for any existing validation patterns for pod name/namespace in the codebase
rg -n "if.*podName|if.*namespace" packages/extension/src/pod-logs/ -B1 -A1Length of output: 0
Let me do one final check to understand when this could occur:
🏁 Script executed:
#!/bin/bash
# Check if object type is V1Pod and verify metadata requirements
rg -n "object:" packages/webview/src/component/pods/PodLogs.svelte -B2 -A2 | head -20Length of output: 86
🏁 Script executed:
#!/bin/bash
# Check imports and type definitions for the object parameter
head -30 packages/webview/src/component/pods/PodLogs.svelteLength of output: 1236
🏁 Script executed:
#!/bin/bash
# Look for any comments or documentation about why empty string fallback is used
rg -n "metadata\?" packages/webview/src/component/pods/PodLogs.svelte -B5 -A5Length of output: 764
Remove unnecessary empty string fallbacks or add early validation with proper error handling.
Verification confirms the concern: while V1Pod objects from the Kubernetes API guarantee metadata.name and metadata.namespace exist, the code defensively provides empty string fallbacks. These empty strings propagate through StreamPodLogs → PodLogsApiImpl → PodLogsService without any validation, eventually reaching the Kubernetes client library's log.log() method. The Kubernetes API would reject empty strings, but errors are only logged to console, providing poor feedback.
Either remove the unnecessary optional chaining (since V1Pod contract guarantees metadata fields exist), or add early validation with user-friendly error messages at lines 160-162.
🤖 Prompt for AI Agents
In packages/webview/src/component/pods/PodLogs.svelte around lines 160-162, the
code supplies empty-string fallbacks for object.metadata.name and
object.metadata.namespace which are unnecessary given V1Pod guarantees and can
lead to poor error propagation; replace the defensive "?? ''" fallbacks by using
the assured properties directly (object.metadata.name,
object.metadata.namespace, containerName), or if you prefer defensive coding add
an early validation block that checks for missing name/namespace and returns or
throws a clear, user-facing error (and log it via the app logger) before calling
StreamPodLogs so empty strings never reach the Kubernetes client.
Signed-off-by: Adameska <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/webview/src/component/pods/PodLogs.svelte (1)
271-276: Simplify the onclick handler for better readability.The explicit boolean return type and returned assignment are unconventional for event handlers. Event handlers don't need to return values in this context.
Consider simplifying to the more idiomatic form:
- onclick={(): boolean => (settingsMenuOpen = !settingsMenuOpen)} + onclick={() => { settingsMenuOpen = !settingsMenuOpen }}or even more concisely:
- onclick={(): boolean => (settingsMenuOpen = !settingsMenuOpen)} + onclick={() => settingsMenuOpen = !settingsMenuOpen}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
packages/webview/src/component/pods/PodLogs.svelte(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build / ubuntu-24.04
- GitHub Check: Build / windows-2022
- GitHub Check: Build / macos-14
- GitHub Check: Build extension OCI image
🔇 Additional comments (4)
packages/webview/src/component/pods/PodLogs.svelte (4)
30-76: Excellent state management and terminal settings synchronization.The reactive state setup is well-structured with clear separation of concerns:
- Persistent preferences (colorfulOutput) saved to localStorage
- Smart synchronization with terminal settings that respects user overrides (lines 70-75 only update tailLines if it hasn't been manually changed)
- Proper initialization from terminalSettingsState
The logic correctly handles the edge case where users might adjust settings independently from the global terminal preferences.
105-183: Robust subscription handling with Promise.allSettled.The refactored
loadLogsfunction is well-designed:
- Proper cleanup of previous subscriptions before establishing new ones
- Good use of
Promise.allSettledto handle multi-container scenarios gracefully—if one container subscription fails, others continue- Clean integration of the
PodLogsOptionsobject to pass all user-configured parameters- Appropriate error logging for failed subscriptions
The approach ensures the UI remains functional even when individual container logs are unavailable.
185-213: Proper lifecycle management and cleanup.The lifecycle hooks correctly:
- Subscribe to terminal settings state and clean up subscriptions on destroy
- Implement the click-outside pattern for the settings menu with proper event listener cleanup
- Clear timeouts and dispose of log subscriptions to prevent memory leaks
The cleanup in the
onMountreturn function andonDestroyensures resources are released properly.
216-337: Well-designed UI with excellent UX patterns.The log viewer UI demonstrates several UX best practices:
- Clear mode selection (Stream/Retrieve) with appropriate button labels
- "Unsynced changes" indicator with helpful tooltip guides users to apply their Tail/Since adjustments
- Settings menu (kebab) provides secondary display preferences without cluttering the main controls
- Proper use of Svelte 5
onclicksyntax for the Button component (line 308)- Accessible labels for all form controls
- Responsive layout with appropriate empty state handling
The distinction between immediate-apply settings (font size, line height, colorful output) and batched-apply controls (tail, since) is intuitive and well-executed.
Signed-off-by: Adameska <[email protected]>
3 participants
…to see previous logs when my pod crashed.
Timestamps can be shown now, and selecting text will now copy it to the clipboard like i would expect (so i can google errors).
BEFORE:
AFTER:
Viewing Previous Logs to see why it crashed:
