feat: Colorize json if detected #485
Conversation
adameska
force-pushed
the
JsonColorize
branch
from
e64f8c0 to
fe5667c
Compare
Signed-off-by: Hettinger, David <DHETTINGER@quad.com>
Signed-off-by: Hettinger, David <DHETTINGER@quad.com>
Signed-off-by: Hettinger, David <DHETTINGER@quad.com>
adameska
force-pushed
the
JsonColorize
branch
from
fe5667c to
92a2f4c
Compare
Signed-off-by: Adameska <hettinger14@outlook.com>
gastoner
left a comment
gastoner
left a comment
There was a problem hiding this comment.
Otherwise it seems like I get some highlighted words on Linux 👍
packages/webview/src/component/terminal/terminal-colors.spec.ts
Outdated
Show resolved
Hide resolved
packages/webview/src/component/terminal/terminal-colors.spec.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Adameska <hettinger14@outlook.com>
Signed-off-by: Adameska <hettinger14@outlook.com>
📝 WalkthroughWalkthroughStreams multi-container pod logs with a subscription-driven pipeline, adds JSON detection and JSON-aware colorization, introduces a ColorOutputType setting and UI, updates fake stream to support multiple subscriptions, adds JSON colorizer utilities and tests, and adjusts terminal styling and resize behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Stream as FakeStreamObject
participant Pod as PodLogs.svelte
participant Detector as detectJsonLogs
participant Formatter as colorizeAndFormatLogs
participant JSON as JsonColorizer
participant Level as colorizeLogLevel
participant Term as TerminalWindow
Stream->>Pod: emit log chunk (pod, namespace, container, text)
Pod->>Pod: route chunk to container subscription
Pod->>Detector: sample/append lines to detect JSON logs
Detector-->>Pod: isJsonFormat? (true/false)
Pod->>Formatter: send chunk + metadata + isJsonFormat
alt JSON detected
Formatter->>JSON: colorize JSON parts
JSON-->>Formatter: JSON-colored text
else Non-JSON
Formatter->>Level: apply level-based coloring / prefixes
Level-->>Formatter: colored text
end
Formatter->>Pod: prepend padded, colored container prefix (if multi-container)
Pod->>Term: write colored, prefixed output
Term-->>Pod: trigger resize/update when new content arrives
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks✅ Passed checks (3 passed)
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: 0
🧹 Nitpick comments (5)
packages/webview/src/component/terminal/json-colorizer.ts (2)
34-211: JSON tokenization logic looks solid with minor limitations around strings and numbersThe character-by-character approach with
handleStructuralChar,processString,processNumber, andprocessKeywordis clear and reasonably robust for log-style JSON. A couple of behavioral edge cases to be aware of:
processStringinfers “key vs value” purely fromisAfterColon, so string elements in arrays (e.g.["a", "b"]) will be treated as keys (isKey = !isAfterColon) and pick upkeyColorif configured, rather thanstringColor. That’s fine for the current default scheme (no key/string colors), but if someone later sets both, array elements will be styled like keys.processNumberonly treats digits,-, and.as part of a number, so JSON numbers with exponents (e.g.1e-5) will only color the leading integer part. Not a blocker, but worth noting if logs can contain such values.If you expect array-heavy or scientific-notation payloads, it may be worth tightening the “is key” detection (e.g., only treat as key when there’s an uncolored colon on the right) and optionally extending number parsing to support
e/Eexponents.
213-254: JSON detection heuristic is reasonable but deliberately excludes some valid JSON shapes
isValidJSON+detectJsonLogsis a pragmatic heuristic for log lines, but its behavior is opinionated:
- It only flags lines as JSON if they contain
{and}and at least one"key": valuepattern. Top-level arrays of primitives (e.g.[1,2,3]) or strings (e.g.["a","b"]) will never be treated as JSON, while arrays of objects (e.g.[{"a":1}]) are accepted.detectJsonLogslooks at the first 10 non-empty lines and uses an 80% threshold. That’s good for performance and avoids scanning thousands of lines, but mixed-format logs (some JSON, some not) will flip the entire stream either on or off.Given your log formats this may be perfectly acceptable, but if you later encounter pods that emit top-level arrays or genuinely mixed formats by container, you may want to extend
isValidJSONto accept array-shaped JSON and/or make the threshold configurable.packages/webview/src/component/pods/PodLogs.svelte (2)
27-32: Defaulting unknown JSON format totruecan interfere with early log-level coloringRight now,
(isJsonFormat ?? true)means:
- Before detection runs (i.e., while
logBuffer.length < 10), all lines are treated as JSON and passed throughcolorizeJSONfirst.- For non-JSON logs that use
[...]level prefixes,colorizeJSONwill wrap the brackets/timestamps in ANSI codes, which can then break the regex patterns incolorizeLogLevelfor those early lines.- Pods that only emit a handful of non-JSON lines (<10 non-empty lines total) will always be treated as JSON and never use the plain
colorizeLogLevelpath.If you want to avoid surprising behavior for short or non-JSON logs, consider flipping the default to “not JSON until proven otherwise”, e.g.:
// Only treat as JSON if detection has explicitly said so lines = isJsonFormat === true ? lines.map(line => colorizeLogLevel(colorizeJSON(line))) : lines.map(line => colorizeLogLevel(line));This will delay JSON colorization until you have enough evidence, at the cost of not JSON-colorizing very short JSON-only logs.
Also applies to: 42-84
51-84: JSON detection is global per pod; mixed-format containers will share the same mode
logBufferandisJsonFormatare shared across all containers in the pod:
- Detection uses the first 10 non-empty lines from all containers combined.
- Once
isJsonFormatis set, it applies to all subsequent lines, regardless of which container they come from.For most setups where “all logs are JSON or none are,” this is fine. But for pods with mixed behavior (e.g., a JSON-logging sidecar and a plain-text main container), a majority format will drive the behavior for all containers, potentially leading to JSON colorization being applied to non-JSON logs or vice versa.
If you anticipate such pods, a future enhancement would be to track
isJsonFormatandlogBufferper containerName (e.g., aMap<string, { isJsonFormat; logBuffer }>), and run detection independently per stream. Not a blocker, but worth considering.Also applies to: 104-111
packages/webview/src/component/terminal/terminal-colors.spec.ts (1)
21-42: Log-level and JSON colorization tests look good; consider tightening ANSI expectationsThe refactor to
test.eachplus the addedcolorizeJSONtest give nice, compact coverage of:
- Multiple log-level formats (brackets, colon, mid-line, quoted JSON levels, case variations, abbreviations).
- Interaction with the new JSON colorization path.
One small nit: some cases use partial ANSI codes like
'\u001b[31'as the expectedcolor, while the implementation actually emits full sequences (e.g.'\u001b[31;1m'). BecausetoContainonly checks substrings, these tests still pass but won’t catch changes to the exact color sequence.If you want stricter protection against regressions, it would be slightly cleaner to:
- Use the full escape sequences in expectations (e.g.
'\u001b[31;1m'), or- Export the color map from
terminal-colors.ts(or a test-only helper) and assert against those constants.Not urgent, but a nice follow-up clean-up.
Also applies to: 55-71, 100-108, 141-164
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/webview/src/component/pods/PodLogs.svelte(5 hunks)packages/webview/src/component/terminal/json-colorizer.spec.ts(1 hunks)packages/webview/src/component/terminal/json-colorizer.ts(1 hunks)packages/webview/src/component/terminal/terminal-colors.spec.ts(4 hunks)packages/webview/src/component/terminal/terminal-colors.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/webview/src/component/terminal/terminal-colors.spec.ts (1)
packages/webview/src/component/terminal/terminal-colors.ts (2)
colorizeLogLevel(75-120)colorizeJSON(145-147)
packages/webview/src/component/terminal/terminal-colors.ts (1)
packages/webview/src/component/terminal/json-colorizer.ts (2)
JsonColorScheme(19-27)JsonColorizer(38-211)
packages/webview/src/component/terminal/json-colorizer.spec.ts (1)
packages/webview/src/component/terminal/json-colorizer.ts (3)
JsonColorScheme(19-27)JsonColorizer(38-211)detectJsonLogs(240-254)
⏰ 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 / ubuntu-24.04
- GitHub Check: Build / macos-14
- GitHub Check: Build / windows-2022
🔇 Additional comments (2)
packages/webview/src/component/terminal/terminal-colors.ts (1)
19-21: Centralized JSON colorization and default scheme look goodThe
defaultJsonColorScheme+ sharedJsonColorizerinstance is a clean way to exposecolorizeJSONwithout leaking scheme details to callers. OmittingkeyColor/stringColorby default matches the tests’ expectation that only braces, numbers, booleans, andnullare colored, keeping JSON readable without overwhelming color use.No changes needed from my side here.
Also applies to: 122-147
packages/webview/src/component/terminal/json-colorizer.spec.ts (1)
24-355: Test coverage for JSON colorization and detection is comprehensiveThe suite does a good job covering:
- Brace/bracket and primitive value coloring (numbers, booleans,
null).- Nested structures, mixed-type arrays, escaped quotes, and custom schemes.
detectJsonLogsbehavior across thresholds, empty/whitespace lines, and non-JSON-with-braces cases.If you ever extend the implementation to support additional shapes, optional extras could be tests for:
- Top-level arrays of primitives/strings as logs.
- Numbers with exponent notation (e.g.
1e-5).But for the current behavior, this coverage looks solid.
Signed-off-by: Adameska <hettinger14@outlook.com>
Signed-off-by: Adameska <hettinger14@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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). (1)
- GitHub Check: Build extension OCI image
🔇 Additional comments (5)
packages/webview/src/component/pods/PodLogs.svelte (5)
9-15: LGTM!The new imports for JSON colorization and terminal color utilities are appropriate for the added functionality.
27-31: LGTM!The state variables for JSON detection and colorization control are well-structured. The TODO comment appropriately documents the future enhancement for user control.
42-58: LGTM!The
addLogPrefixfunction correctly handles container name prefixing with proper padding and color application. The condition on line 53 appropriately prevents adding prefixes to trailing empty lines.
101-109: LGTM!The
calculatePrefixLengthfunction correctly determines the maximum container name length for proper prefix alignment in multi-container pods.
124-155: LGTM!The
onMountlifecycle correctly orchestrates the log streaming setup:
- Properly handles both single and multi-container pods
- Uses tick() before dispatching resize to ensure DOM updates complete
- Correctly manages disposables for cleanup
The centralized formatting through
colorizeAndFormatLogsstreamlines the implementation while maintaining multi-container support.
Signed-off-by: Adameska <hettinger14@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/webview/src/component/pods/PodLogs.svelte (2)
60-93: Revisit JSON detection default and short‑log behavior.While
isJsonFormatisundefined, you still default to JSON colorization via(isJsonFormat ?? true). Combined with the 10‑line buffer, this means:
- For the first 9 non‑empty lines, JSON colorization is always attempted even before detection runs.
- For pods that emit fewer than 10 non‑empty lines total,
isJsonFormatnever gets set, so you permanently treat logs as JSON.This matches the earlier concern but hasn’t changed. Consider instead:
- Defaulting to non‑JSON until detection decides (
isJsonFormat ?? false), or- Only invoking
colorizeJSONwhenisJsonFormat === true, leaving everything as plain text + log‑level coloring while detection is pending.For example:
- lines = - (isJsonFormat ?? true) - ? lines.map(line => colorizeLogLevel(colorizeJSON(line))) - : lines.map(line => colorizeLogLevel(line)); + lines = + isJsonFormat === true + ? lines.map(line => colorizeLogLevel(colorizeJSON(line))) + : lines.map(line => colorizeLogLevel(line));If the current “JSON‑first” behavior is intentional, it would help to tighten the comment to spell that out and maybe make the buffer size (
10) a named constant so the heuristic is easier to reason about.
111-122: Fix the JSDoc forsetupPrefixColoursto describe color mapping, not prefix length.The JSDoc here still duplicates the description for
calculatePrefixLengthand doesn’t explain that this function initializes the ANSI color mapping for container prefixes. This matches a prior review comment and is still unresolved.Consider updating it along these lines:
/** - * Calculates the maximum container name length for padding prefixes in multi-container - * pods so that log lines align correctly. - * - * @returns Max container name length for prefix padding - */ + * Sets up ANSI 256 colour mappings for container name prefixes in multi-container + * pods. Cycles through the available colours when there are more containers than + * colours and stores the formatted (colourized) names in `prefixColourMap`. + */This will make the intent much clearer to future readers.
🧹 Nitpick comments (3)
packages/webview/src/component/pods/PodLogs.svelte (3)
38-58: MakeaddLogPrefixresilient to missing prefix colors and clarify assumptions.
addLogPrefixassumesprefixColourMapalways contains an entry forprefix; if it doesn’t, the rendered output becomesundefined|…. Today this is safe becausesetupPrefixColoursruns before subscriptions and we only calladdLogPrefixwith container names fromobject.spec?.containers, but this helper is generic and could be re‑used incorrectly later.Consider a small defensive tweak to fall back to the raw prefix if no color is found, and optionally document the expectation that
setupPrefixColoursmust have run:- const colouredName = prefixColourMap.get(prefix); + const colouredName = prefixColourMap.get(prefix) ?? prefix;This keeps output sane even if the color map isn’t initialized or a new prefix is introduced.
60-93: AlignshouldColorizeLogssemantics with all colorization paths (including prefixes).Right now
shouldColorizeLogsgates JSON + log‑level coloring, but container prefixes are still colorized unconditionally viaaddLogPrefixusingprefixColourMap(built withcolourizedANSIContainerName). Once you wireshouldColorizeLogsto a UI toggle, users might reasonably expect all ANSI coloring to turn off.You could either:
- Gate prefix coloring on
shouldColorizeLogsas well (e.g., pass a flag intoaddLogPrefix), or- Leave prefixes colored but capture that explicitly in the variable name or a short comment.
This is non‑blocking today since the toggle isn’t exposed yet, but worth deciding before the setting becomes user‑visible.
124-155: Multi‑container behavior preserved; consider throttling resize events if logs are very chatty.The refactored
onMountkeeps the multi‑container feature: you compute a sharedmaxNameLength, set up per‑container prefix colors, and then subscribe once per container with a consistentlogLinePrefix. The integration withcolorizeAndFormatLogslooks coherent.One minor consideration: the
tick().then(() => window.dispatchEvent(new Event('resize')))call runs for every log chunk, across all containers. For very high‑volume logs this could generate a lot of resize events. If you see performance issues in practice, consider debouncing or throttling this dispatch.Otherwise, the lifecycle wiring and disposables management look solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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 / macos-14
- GitHub Check: Build / windows-2022
- GitHub Check: Build extension OCI image
🔇 Additional comments (1)
packages/webview/src/component/pods/PodLogs.svelte (1)
95-109:calculatePrefixLengthmatches intent and keeps multi‑container alignment.The function correctly computes a max container name length with null‑safe access via
object.spec?.containers, returning0for single‑ or zero‑container pods. That integrates cleanly with the latermaxNameLength > 0 ? containerName : undefinedlogic.No changes needed here.
Signed-off-by: Adameska <hettinger14@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/webview/src/component/pods/PodLogs.svelte (1)
27-32: JSON detection currently assumes JSON before detection completes
isJsonFormatstarts asundefined, but the colorization branch uses(isJsonFormat ?? true), so logs are treated as JSON until detection explicitly setsfalse. This can:
- Miscolor non‑JSON logs when the stream ends before 10 non‑empty lines are seen
- Conflict with the “JSON if detected” semantics from the PR description
To align behavior with “detected JSON” and avoid surprising formatting, consider only enabling JSON colorization once detection has confirmed it:
- // Apply colorization: JSON first (if detected/only a few lines of logs), then log levels - lines = - (isJsonFormat ?? true) - ? lines.map(line => colorizeLogLevel(colorizeJSON(line))) - : lines.map(line => colorizeLogLevel(line)); + // Apply colorization: JSON first (once detected), then log levels + const useJsonColorization = isJsonFormat === true; + lines = useJsonColorization + ? lines.map(line => colorizeLogLevel(colorizeJSON(line))) + : lines.map(line => colorizeLogLevel(line));If you intentionally want “assume JSON until proven otherwise”, I’d at least update the comment to spell that out so future readers aren’t surprised by the
?? truedefault.Also applies to: 72-87
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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). (1)
- GitHub Check: Build extension OCI image
🔇 Additional comments (2)
packages/webview/src/component/pods/PodLogs.svelte (2)
95-120: Prefix utilities and JSDoc now match behavior
calculatePrefixLengthandsetupPrefixColourslook consistent with the multi‑container requirements:
calculatePrefixLengthcorrectly computes the max container name length, guarding via optional chaining.setupPrefixColourscycles throughansi256Coloursand usescolourizedANSIContainerNameto build stable, colored prefixes per container.- The updated JSDoc for
setupPrefixColoursnow accurately documents the color‑mapping behavior and resolves the earlier duplication issue.No changes needed here.
122-149: I'm unable to verify the review comment because the repository cannot be cloned in the sandbox environment. Without access to the codebase, I cannot examine:
- The
colorizeAndFormatLogsfunction implementation and how it handles line endings- The xterm terminal configuration and whether
convertEolis actually enabled- Other instances of manual CR handling in the Pod logs code
- The actual chunk processing flow
The review comment suggests a refactoring to remove the
'\r'suffix and letconvertEolhandle line endings entirely. This is a reasonable concern about EOL handling, but verification requires direct code inspection which I cannot perform without repository access.Re-check EOL handling with formatted logs +
convertEolWith the new formatting flow you now:
- Split/join chunks with
'\n'incolorizeAndFormatLogs- Then write
formattedLogs + '\r'to an xterm terminal that already hasconvertEolenabledDepending on the exact chunk shapes, this can lead to an extra bare
'\r'at the end of each write, which may cause subtle cursor behavior (e.g. carriage‑return without newline).You may want to simplify this to let
convertEolhandle line endings entirely:- const formattedLogs = colorizeAndFormatLogs(chunk.data, logLinePrefix, maxNameLength); + const formattedLogs = colorizeAndFormatLogs(chunk.data, logLinePrefix, maxNameLength); if (noLogs) { noLogs = false; } - logsTerminal?.write(formattedLogs + '\r'); + logsTerminal?.write(formattedLogs);Worth quickly testing multi‑line logs before/after this change to confirm there's no regression in how lines wrap and scroll.
…nt that makes enough sense to me... Signed-off-by: Adameska <hettinger14@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/webview/src/component/pods/PodLogs.svelte (1)
80-84: JSON detection default behavior concern was previously raised.The behavior of
isJsonFormat ?? truecausing JSON colorization to be applied before detection completes was flagged in a prior review. If this is intentional (to colorize JSON even with few log lines), consider updating the comment for clarity.
🧹 Nitpick comments (2)
packages/webview/src/component/pods/PodLogs.svelte (2)
99-106: Consider simplifying withMath.maxandmap.The loop can be replaced with a more concise expression.
const calculatePrefixLength = (): number => { - let maxNameLength = 0; - object.spec?.containers.forEach(container => { - if (container.name.length > maxNameLength) { - maxNameLength = container.name.length; - } - }); - return maxNameLength; + const containers = object.spec?.containers ?? []; + return containers.length > 0 + ? Math.max(...containers.map(c => c.name.length)) + : 0; };
142-146: Consider debouncing resize events for heavy log streams.Dispatching a resize event after every log chunk could cause performance overhead with high-volume logging. Consider debouncing this to limit resize events to a reasonable frequency (e.g., once every 100-200ms).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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 / ubuntu-24.04
- GitHub Check: Build / windows-2022
- GitHub Check: Build / macos-14
🔇 Additional comments (2)
packages/webview/src/component/pods/PodLogs.svelte (2)
42-56: Prefix handling now properly hardened.The defensive changes from previous feedback are implemented correctly: early return for missing prefix/empty lines, safe padding calculation with
Math.max(0, ...), and fallback for missing color mappings.
120-151: Overall refactoring approach looks good.The centralized
colorizeAndFormatLogsfunction with prefix handling provides a clean separation of concerns. The multi-container support with per-container prefixes and color mapping is well-structured.
Signed-off-by: Adameska <hettinger14@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/webview/src/component/pods/PodLogs.svelte (1)
56-98: Tighten JSON sampling window and simplify line splittingThe overall formatting pipeline looks good, but the JSON sampling can overshoot the intended sample size when multiple chunks arrive before the buffer is full:
- When
logBuffer.lengthis already > 0 but< jsonColorizeSampleSize, you still slice up tojsonColorizeSampleSizefrom the current chunk, sologBuffercan grow beyond 20 lines. It’s harmless functionally (more data for detection) but diverges from the configured sample size and makes tuning harder.You can clamp to the remaining capacity like this:
- // Auto-detect JSON format from first batch of logs, keep checking until we have processed enough lines to determine if the log is most likely formatted in JSON - if (logBuffer.length < jsonColorizeSampleSize) { - logBuffer.push(...lines.filter(l => l.trim()).slice(0, jsonColorizeSampleSize)); - if (logBuffer.length > 0) { - isJsonFormat = detectJsonLogs(logBuffer); - } - } + // Auto-detect JSON format from the first jsonColorizeSampleSize non-empty lines. + if (logBuffer.length < jsonColorizeSampleSize) { + const remainingCapacity = jsonColorizeSampleSize - logBuffer.length; + if (remainingCapacity > 0) { + const nonEmptyLines = lines.filter(l => l.trim()).slice(0, remainingCapacity); + logBuffer.push(...nonEmptyLines); + } + if (logBuffer.length > 0) { + isJsonFormat = detectJsonLogs(logBuffer); + } + }As an optional cleanup, you could also split
dataonce and reuse it for both colorization and prefix handling (instead of re-splitting whenshouldColorizeLogsis false butprefixis set), which would let you drop theif (lines.length === 0)fallback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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 (3)
packages/webview/src/component/pods/PodLogs.svelte (3)
40-54: Prefix padding and color mapping look robustThe prefix helper is nicely defensive:
Math.max(0, prefixLength - prefix.length)prevents negative repeat counts.prefixColourMap.get(prefix) ?? prefixkeeps output sane even if colors weren’t initialized.- Skipping the final empty line avoids spurious prefixed blank rows.
Multi-container alignment and fallback behavior look good to me.
100-125: Prefix helper utilities are clear and match their JSDoc
calculatePrefixLengthandsetupPrefixColoursdo exactly what their comments describe:
- Safely handle missing
object.spec?.containers.- Use the maximum container name length for padding.
- Cycle through
ansi256Colourswith modulo to assign stable colors per container.No issues spotted here.
127-158: onMount wiring cleanly integrates the new formatting pipelineThe on-mount logic is straightforward:
- Clears the terminal and computes
maxNameLength/prefix colors only for multi-container pods.- Subscribes once per container and routes chunks through
colorizeAndFormatLogswith the appropriate prefix.- Keeps the existing
noLogsbehavior and resize-on-chunk pattern.This keeps multi-container behavior intact while centralizing formatting, which should make future tweaks (e.g., exposing
shouldColorizeLogsvia a toolbar) easier.
@adameska, In my opinion, it would be better for the user to have the choice in the coloring scheme (none, standard, json), with a conservative default (or setting a default in Preferences for all pods; for the moment, Podman Desktop extensions do not support saving preferences per container/image/pod/etc). Perhaps can you introduce the toolbar as part of this PR? Or introduce it in a separate PR with a simple configuration (for example a setting which would be the same for all the containers of the pod, either the Timestamp prefix or the Since value?) |
|
The color settings are a good idea, and it would be great to expose them in the Preferences. That could work really well, and wouldn't need any new UI (besides the dropdown in Preferences). |
Say the word and i'll merge these changes into my other branch and kill this branch off. I will add a dropdown (right now i have a checkbox) in that other branch for coloring: none, basic, full. Caching preference by podname is probably a bad idea since that should change all the time but if i could get at the deployment name for the pod then i could cache your preferences for coloring by that. Let me know which way you would like to go! |
No, please don't merge this branch with the other one, as it will make a branch with too much changes to review. The idea is to have changes as smallest as possible, to ease the review, validation, etc. But I think that you could create the toolbar into this PR, with only the color selection
In my opinion, the best for the moment is to have a global setting in the Preferences of the extension, with the possibility to change for each pod in the toolbar, without caching the preference for the moment. We can introduce this caching later when we have discussed a lobal pattern with the team for Podman Desktop and extensions. |
Signed-off-by: Adameska <hettinger14@outlook.com>
Ok, i've merged a fair amount of the toolbar logic over from my other PR to make merge conflicts easier once this gets completed. Please let me know if this is acceptable (for now i'm caching the users selection globally of their color preferences in this window but again, could add deployment name to that color setting so it's per deployment). |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/webview/src/component/pods/PodLogs.svelte (1)
77-85: Buffer can exceed sample size.The slice on line 79 always takes up to
jsonColorizeSampleSizelines without accounting for lines already in the buffer. IflogBufferhas 15 lines and the next chunk has 10 lines, the buffer will grow to 25 lines instead of capping at 20.const isJsonDetected = (lines: string[]): boolean => { if (logBuffer.length < jsonColorizeSampleSize) { - logBuffer.push(...lines.filter(l => l.trim()).slice(0, jsonColorizeSampleSize)); + const remainingSlots = jsonColorizeSampleSize - logBuffer.length; + logBuffer.push(...lines.filter(l => l.trim()).slice(0, remainingSlots)); if (logBuffer.length > 0) { jsonDetected = detectJsonLogs(logBuffer); } } return jsonDetected; };
🧹 Nitpick comments (2)
packages/webview/src/component/pods/PodLogs.spec.ts (1)
145-162: Consider verifying padding in this multi-container test.The test verifies the colored prefix format but doesn't check for padding alignment. The container names
cnt1(4 chars) andcontainerName2(14 chars) should result in padding forcnt1. Consider adding explicit padding verification similar to the test on lines 252-269.const writtenLog = vi.mocked(mockedTerminal.write).mock.calls[0][0] as string; - expect(writtenLog).toContain('\u001b[36mcnt1\u001b[0m|log from cnt1'); + // 'containerName2' is 14 chars, 'cnt1' is 4 chars, so expect 10 spaces of padding + const tenSpaces = ' '; + expect(writtenLog).toContain(tenSpaces + '\u001b[36mcnt1\u001b[0m|log from cnt1');packages/webview/src/component/pods/PodLogs.svelte (1)
225-234: Consider using a string sentinel instead ofundefinedfor the "Auto" option.Binding
undefinedto a<select>option value can be unreliable across browsers. Consider using an empty string or a specific string like'auto'as the sentinel value.- <option value={undefined} class="bg-(--pd-content-card-bg)">Auto</option> + <option value="" class="bg-(--pd-content-card-bg)">Auto</option>And update the state initialization and effect accordingly:
-let colorfulOutputType = $state(localStorage.getItem(colorfulOutputCacheKey) ?? undefined); +let colorfulOutputType = $state(localStorage.getItem(colorfulOutputCacheKey) ?? '');- if (colorfulOutputType === undefined) { + if (colorfulOutputType === '') {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/webview/src/component/pods/PodLogs.spec.ts(2 hunks)packages/webview/src/component/pods/PodLogs.svelte(3 hunks)packages/webview/src/component/terminal/TerminalWindow.svelte(1 hunks)packages/webview/src/component/terminal/color-output-types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/webview/src/component/terminal/color-output-types.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/webview/src/component/pods/PodLogs.spec.ts (3)
packages/rpc/src/rpc.ts (1)
name(272-274)packages/channels/src/model/pod-logs-chunk.ts (1)
PodLogsChunk(19-24)packages/channels/src/channels.ts (1)
API_POD_LOGS(66-66)
⏰ 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 extension OCI image
- GitHub Check: Build / windows-2022
🔇 Additional comments (13)
packages/webview/src/component/terminal/TerminalWindow.svelte (1)
148-152: LGTM!Adding
h-full w-fullensures the terminal properly fills its container, which is necessary for correct xterm.js rendering and integrates well with the flex layout in the parentPodLogscomponent.packages/webview/src/component/pods/PodLogs.spec.ts (5)
39-67: Good test helper organization.The helper functions
createMockTerminal,createPod, andsetupTerminalMockeffectively reduce duplication and improve test readability. The approach of binding the terminal through the mockedTerminalWindowprops is appropriate for testing the integration.
69-79: Clean test setup.The
beforeEachproperly resets all mocks and clears localStorage to ensure test isolation for the colorful output preference tests.
81-122: Solid coverage for EmptyScreen behavior.The parameterized test for multiple container configurations and the verification that
EmptyScreenis not re-rendered after logs arrive effectively test the conditional rendering logic.
252-269: Good fix for the padding verification.The test now explicitly verifies the padding with
equals()instead oftoContain(), ensuring the exact format is validated. The 9-space calculation correctly accounts for the difference between 'longername' (10 chars) and 'a' (1 char).
336-378: Comprehensive multi-container color test.The use of
waitForSubscriptions(3)properly handles the async nature of container subscription setup, and the sequential log sending ensures deterministic verification of the color cycling behavior.packages/webview/src/component/pods/PodLogs.svelte (7)
27-44: Well-structured state management.The state variables are clearly named and the separation between persistent settings (
colorfulOutputTypevia localStorage) and runtime state (jsonDetected,noLogs) is appropriate. UsingSvelteMapforprefixColourMapensures proper Svelte reactivity.
46-63: Appropriate reactive effects.The localStorage synchronization correctly handles the undefined case by removing the key, and triggering resize when logs appear ensures proper terminal rendering.
65-75: Defensive implementation addresses prior concerns.The use of
Math.max(0, ...)prevents negative repeat counts, and the fallbackprefixColourMap.get(prefix) ?? prefixensures graceful handling of missing color mappings.
96-114: Verify BASIC mode behavior.When
colorfulOutputTypeisColorOutputType.BASIC, the condition on line 103 evaluates tofalse, so onlycolorizeLogLevelis applied (notcolorizeJSON). Ensure this is the intended behavior for BASIC mode.
164-188: Robust subscription handling with graceful degradation.Using
Promise.allSettledensures that a failure in one container's log subscription doesn't prevent other containers from being subscribed. Error logging for failed subscriptions provides visibility without disrupting the user experience.
190-206: Clean lifecycle management.The
onMountproperly initializes log loading and the click-outside handler with cleanup, whileonDestroyensures proper disposal of subscriptions.
242-252: Good approach to toggle visibility while preserving state.Using
invisibleandh-0classes instead of conditional rendering keeps theTerminalWindowmounted, which preserves terminal state and avoids re-initialization when logs appear.
|
I have created the epic #524 to follow your changes in the logs page. To be sure that all concerned people can review each change, and to include changes incrementally, it would be preferable that we work on separate PRs for each item of the epic, which are describing steps small enough to facilitate reviews and integration. |
Signed-off-by: Adameska <hettinger14@outlook.com>
I do have a fulltime other job. I've been adding these features in my spare time because i love to work with podman but it's log features are quite lacking over some of the other tools i use so i figured i would contribute. It is getting time consuming splitting up these small changes. Splitting up this PR to remove to have a toolbar in a separate PR seems pretty useless to me... what is it going to control if we don't have this json feature (i thought that was the whole point of adding one)? If we don't like auto-detection I can just default to your choice and remove that and Json detection, but it worked well for the few containers i use and allowed me not to have to constantly flip this setting (which will matter more if it doesn't remember what i pick). |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/webview/src/component/pods/PodLogs.svelte (1)
68-76: Limit JSON detection buffer to the configured sample size
isJsonDetectedcan overshootjsonColorizeSampleSize: iflogBuffer.lengthis 19 and you receive 20 new non-empty lines,slice(0, jsonColorizeSampleSize)will push all 20, leavinglogBuffer.length === 39. That means you’re no longer sampling “the first 20 non-empty lines”.You can clamp to the remaining budget per call:
const isJsonDetected = (lines: string[]): boolean => { - if (logBuffer.length < jsonColorizeSampleSize) { - logBuffer.push(...lines.filter(l => l.trim()).slice(0, jsonColorizeSampleSize)); - if (logBuffer.length > 0) { - jsonDetected = detectJsonLogs(logBuffer); - } - } + if (logBuffer.length < jsonColorizeSampleSize) { + const remaining = jsonColorizeSampleSize - logBuffer.length; + const newLines = lines.filter(l => l.trim()).slice(0, remaining); + if (newLines.length > 0) { + logBuffer.push(...newLines); + jsonDetected = detectJsonLogs(logBuffer); + } + } return jsonDetected; };That keeps detection bounded to the intended sample window while preserving the incremental behavior you added.
🧹 Nitpick comments (5)
packages/webview/src/component/pods/PodLogs.svelte (5)
27-32: Consider typingcolorfulOutputTypeexplicitly for stronger TS checksThe initial state wiring (
noLogs,colorfulOutputType,jsonDetected,settingsMenuOpen,logBuffer,settingsMenuRef) is sensible and safe. To tighten typing around the select binding and comparisons, you could explicitly parameterize the rune state:-const jsonColorizeSampleSize = 20; - -let noLogs = $state(true); -let colorfulOutputType = $state(undefined); -let jsonDetected = $state(false); +const jsonColorizeSampleSize = 20; + +let noLogs = $state<boolean>(true); +let colorfulOutputType = $state<ColorOutputType | undefined>(undefined); +let jsonDetected = $state<boolean>(false);This helps catch accidental assignments outside the intended enum/undefined domain.
Also applies to: 35-37
41-44: Multi-container prefix alignment and color mapping look solidThe combination of
calculatePrefixLength,setupPrefixColours, andaddLogPrefixpreserves the multi-container feature while hardening padding and color lookup:
- Safe padding via
Math.max(0, prefixLength - prefix.length)avoids.repeaterrors.- Fallback to the raw prefix when
prefixColourMaphas no entry preventsundefined|...artifacts.- Prefix is only applied when
maxNameLength > 0, so single-container pods are unaffected.One tiny optional improvement would be to clear the map when recomputing colors, in case container sets change within a single component instance:
const setupPrefixColours = (): void => { - object.spec?.containers.forEach((container, index) => { + prefixColourMap.clear(); + object.spec?.containers.forEach((container, index) => { const colour = ansi256Colours[index % ansi256Colours.length]; prefixColourMap.set(container.name, colourizedANSIContainerName(container.name, colour)); }); };But as-is, behavior is correct for stable container lists and keeps multi-container prefixes intact.
Also applies to: 56-66, 107-132, 149-153, 155-169
78-105: Consider preserving prefixes whenColorOutputType.NONEis selectedRight now,
ColorOutputType.NONEshort-circuits and returns the rawdatastring:if (colorfulOutputType === ColorOutputType.NONE || (data?.length ?? 0) === 0) { return data; }This means in multi-container pods, choosing “None” removes both colors and the container prefixes, which may make it harder to distinguish containers even though only colorization was disabled.
If the intent is “no color but keep structural formatting”, you could restructure:
const colorizeAndFormatLogs = (data: string, prefix?: string, maxPrefixLength: number = 0): string => { - if (colorfulOutputType === ColorOutputType.NONE || (data?.length ?? 0) === 0) { - return data; - } - - let lines: string[] = data.split('\n'); - //format json if the user asked for that or if they didn't set a preference and we auto-detected json logs - let isJsonFormat = colorfulOutputType === ColorOutputType.FULL || (!colorfulOutputType && isJsonDetected(lines)); - - lines = isJsonFormat - ? lines.map(line => colorizeLogLevel(colorizeJSON(line))) - : lines.map(line => colorizeLogLevel(line)); - - if (prefix) { - addLogPrefix(lines, prefix, maxPrefixLength); - } - - return lines.join('\n'); + if ((data?.length ?? 0) === 0) { + return data; + } + + let lines: string[] = data.split('\n'); + + if (colorfulOutputType === ColorOutputType.NONE) { + if (prefix) { + addLogPrefix(lines, prefix, maxPrefixLength); + } + return lines.join('\n'); + } + + // format json if the user asked for that or if they didn't set a preference and we auto-detected json logs + const isJsonFormat = + colorfulOutputType === ColorOutputType.FULL || (!colorfulOutputType && isJsonDetected(lines)); + + lines = isJsonFormat + ? lines.map(line => colorizeLogLevel(colorizeJSON(line))) + : lines.map(line => colorizeLogLevel(line)); + + if (prefix) { + addLogPrefix(lines, prefix, maxPrefixLength); + } + + return lines.join('\n'); };If “None” is intentionally “raw stream, no prefixes or colors”, this can be left as-is, but it’s worth double-checking the UX expectation.
52-54: Avoid overlappingloadLogsexecutions creating duplicate subscriptions
handleSettingChangefiresloadLogs().catch(console.error)without awaiting it, andloadLogsitself is async. Rapidly toggling the dropdown could start multipleloadLogscalls concurrently:
- The second call clears
disposablesbefore the first call’sPromise.allSettledhas populated it.- Subscriptions created by the first
loadLogsmay stay active until a later reload/onDestroy, potentially leading to duplicated log writes for a while.You could serialize reloads or guard against stale runs, for example:
-let disposables: IDisposable[] = []; +let disposables: IDisposable[] = []; +let loadGeneration = 0; @@ async function loadLogs(): Promise<void> { + const myGen = ++loadGeneration; disposables.forEach(disposable => disposable.dispose()); disposables = []; @@ - const results = await Promise.allSettled(subscriptionPromises); + const results = await Promise.allSettled(subscriptionPromises); + + if (myGen !== loadGeneration) { + // A newer loadLogs call has started; ignore these subscriptions. + results.forEach(result => { + if (result.status === 'fulfilled') { + result.value.dispose(); + } + }); + return; + }This keeps at most one active subscription set at a time regardless of how often settings are toggled.
Also applies to: 142-179
200-231: Settings dropdown wiring is clear; consider small accessibility polishThe settings bar and dropdown are wired cleanly:
- Ellipsis button with an icon and
aria-labelopens/closes the menu.- The “Colorful Output” select binds directly to
colorfulOutputTypeand callshandleSettingChangeon change, matching the state logic above.For a minor a11y improvement, you might add
aria-haspopup="menu"andaria-expanded={settingsMenuOpen}to the button, and possiblyrole="menu"on the menu container:- <button - class="p-2 hover:bg-(--pd-content-card-hover-bg) rounded" - onclick={(): boolean => (settingsMenuOpen = !settingsMenuOpen)} - aria-label="Terminal settings"> + <button + class="p-2 hover:bg-(--pd-content-card-hover-bg) rounded" + onclick={(): boolean => (settingsMenuOpen = !settingsMenuOpen)} + aria-label="Terminal settings" + aria-haspopup="menu" + aria-expanded={settingsMenuOpen}> @@ - <div + <div + role="menu" class="absolute right-0 mt-1 w-64 bg-(--pd-content-card-bg) border border-(--pd-content-divider) rounded shadow-lg z-10">Not required for functionality, but it would make the control more screen-reader friendly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/webview/src/component/pods/PodLogs.svelte(3 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 / windows-2022
- GitHub Check: Build / ubuntu-24.04
- GitHub Check: Build extension OCI image
- GitHub Check: Build / macos-14
🔇 Additional comments (2)
packages/webview/src/component/pods/PodLogs.svelte (2)
2-18: Imports and colorization utilities are wired in cleanlyThe new imports for
Fa,faEllipsisV,ColorOutputType, JSON detection, and terminal color utilities are cohesive and match their usage below; no issues here.
45-50: Resize effect, click-outside handling, and terminal visibility behave coherently
$effectkeyed onnoLogsplustriggerResize(usingtickand aresizeevent) gives xterm a chance to recalc size only when logs first arrive or after a reload.- The
onMounthook correctly wires a window click listener and removes it via the returned cleanup, so the settings menu closes on outside click without leaking listeners.- The
noLogs-driven conditional rendering and class bindings (invisible,h-0,flex-1) cleanly switch between theEmptyScreenand a full-heightTerminalWindow.This wiring looks consistent and robust.
Also applies to: 134-140, 181-191, 233-243
@vancura wdyt about the toolbar introduced in the logs page? |
Signed-off-by: Adameska <hettinger14@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/webview/src/component/pods/PodLogs.spec.ts (1)
182-205: Test still relies onlocalStorageeven though the component doesn’tIn
test('should apply JSON colorization when colorfulOutputType is FULL', ...)(Lines 182‑205),localStorage.setItem('podlogs.terminal.colorful-output', 'full')no longer influencesPodLogssince the component only uses the in‑componentcolorfulOutputTypestate and the settings dropdown.As written, the test passes because Auto mode plus
isJsonDetectedtriggers JSON colorization, not because FULL was set via storage. This makes the test name and setup misleading, and it leaves a straylocalStoragedependency despite the earlier request to avoid it.Consider either:
- Driving FULL via the settings select (like the
should apply FULL colorization when set, regardless of contenttest) and droppinglocalStorage, or- Renaming this test to something like “auto‑detected JSON colorization” and removing the
localStoragewrite so it reflects current behavior.packages/webview/src/component/pods/PodLogs.svelte (3)
68-76: Clamp JSON sampling to the remaining budget instead of a flat 20 per chunk
isJsonDetectedcurrently does:if (logBuffer.length < jsonColorizeSampleSize) { logBuffer.push(...lines.filter(l => l.trim()).slice(0, jsonColorizeSampleSize)); if (logBuffer.length > 0) { jsonDetected = detectJsonLogs(logBuffer); } }If
logBufferalready contains some lines (e.g., 10) and a new chunk brings 20 more, you end up sampling 30 lines (10 + 20) even thoughjsonColorizeSampleSizeis 20. That makes the “sample size” a soft hint rather than a real cap.If you want a strict upper bound and more predictable detection behavior, slice based on the remaining capacity:
- if (logBuffer.length < jsonColorizeSampleSize) { - logBuffer.push(...lines.filter(l => l.trim()).slice(0, jsonColorizeSampleSize)); + if (logBuffer.length < jsonColorizeSampleSize) { + const remaining = jsonColorizeSampleSize - logBuffer.length; + const nonEmpty = lines.filter(l => l.trim()); + logBuffer.push(...nonEmpty.slice(0, remaining)); if (logBuffer.length > 0) { jsonDetected = detectJsonLogs(logBuffer); } }This keeps the sampling window truly bounded at
jsonColorizeSampleSizenon‑empty lines while preserving the current detection semantics.
87-105:ColorOutputType.NONEdisables prefixes as well as colorizationIn
colorizeAndFormatLogs, this early return:if (colorfulOutputType === ColorOutputType.NONE || (data?.length ?? 0) === 0) { return data; }short‑circuits all formatting, including prefixing:
if (prefix) { addLogPrefix(lines, prefix, maxPrefixLength); }So in multi‑container pods, switching “Colorful Output” to None removes container prefixes entirely — logs become raw Kubernetes output with no way to tell which container they came from.
If the intention is “no colors, but keep multi‑container prefixes” (which seems consistent with wanting to “keep the multicontainers feature”), you could decouple prefixing from colorization, e.g.:
-const colorizeAndFormatLogs = (data: string, prefix?: string, maxPrefixLength: number = 0): string => { - if (colorfulOutputType === ColorOutputType.NONE || (data?.length ?? 0) === 0) { - return data; - } - - let lines: string[] = data.split('\n'); - // format json... - let isJsonFormat = colorfulOutputType === ColorOutputType.FULL || (!colorfulOutputType && isJsonDetected(lines)); - - lines = isJsonFormat - ? lines.map(line => colorizeLogLevel(colorizeJSON(line))) - : lines.map(line => colorizeLogLevel(line)); - - if (prefix) { - addLogPrefix(lines, prefix, maxPrefixLength); - } - return lines.join('\n'); -}; +const colorizeAndFormatLogs = (data: string, prefix?: string, maxPrefixLength: number = 0): string => { + if ((data?.length ?? 0) === 0) { + return data; + } + + let lines = data.split('\n'); + + // Only apply colors when not explicitly disabled + if (colorfulOutputType !== ColorOutputType.NONE) { + const isJsonFormat = + colorfulOutputType === ColorOutputType.FULL || (!colorfulOutputType && isJsonDetected(lines)); + + lines = isJsonFormat + ? lines.map(line => colorizeLogLevel(colorizeJSON(line))) + : lines.map(line => colorizeLogLevel(line)); + } + + if (prefix) { + addLogPrefix(lines, prefix, maxPrefixLength); + } + + return lines.join('\n'); +};You could also adjust
addLogPrefixto avoid ANSI codes whenColorOutputType.NONEis selected (e.g., use the rawprefixinstead ofprefixColourMap), so “None” means “no colors at all, but still prefixed for multi‑container pods”.
123-133: Consider clearingprefixColourMapwhen reloading logs
setupPrefixColoursalwayssets entries inprefixColourMap, but the map is never cleared. Within a long‑lived component whereloadLogsmight be called multiple times (e.g., user toggling settings or switching pods without recreating the component), this can leave stale entries for containers that no longer exist.It probably won’t blow up anything (lookups are keyed by current container names), but you can keep things cleaner and more predictable by clearing before repopulating:
const setupPrefixColours = (): void => { - object.spec?.containers.forEach((container, index) => { + prefixColourMap.clear(); + object.spec?.containers.forEach((container, index) => { const colour = ansi256Colours[index % ansi256Colours.length]; prefixColourMap.set(container.name, colourizedANSIContainerName(container.name, colour)); }); };This keeps the mapping scoped to the current pod’s container set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/webview/src/component/pods/PodLogs.spec.ts(3 hunks)packages/webview/src/component/pods/PodLogs.svelte(3 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 / macos-14
- GitHub Check: Build / windows-2022
- GitHub Check: Build / ubuntu-24.04
- GitHub Check: Build extension OCI image
|
It's fine! For now, the button looks a bit lonely, but since we have the toolbar area, we now have a place to add the new feature (and as I saw elsewhere, they're coming). The wording is a bit complicated. Instead of /cc @MariaLeonova What do you think? |
"Colorize Logs" better? Highlight is a little weird to me since i would think that would mean to highlight the text (background colors). But whatever you guys think is better i'm fine with. Let me know if any changes are needed. This an always get refined in the other PR when i merge with that toolbar... |
|
"Colorize Logs" sounds fine to me! :) |
Signed-off-by: Adameska <hettinger14@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/webview/src/component/pods/PodLogs.svelte (1)
68-76: Fix slice logic and prevent memory leak.Two critical issues:
Slice doesn't respect remaining sample budget: Line 70 always slices up to
jsonColorizeSampleSize(20 lines) from each chunk, not the remaining lines needed. If 15 lines are already in the buffer, it should only take 5 more.Memory leak: The buffer is never cleared after detection completes, so it grows indefinitely.
Apply this diff:
const isJsonDetected = (lines: string[]): boolean => { if (logBuffer.length < jsonColorizeSampleSize) { - logBuffer.push(...lines.filter(l => l.trim()).slice(0, jsonColorizeSampleSize)); + const remainingBudget = jsonColorizeSampleSize - logBuffer.length; + logBuffer.push(...lines.filter(l => l.trim()).slice(0, remainingBudget)); if (logBuffer.length > 0) { jsonDetected = detectJsonLogs(logBuffer); } + // Clear buffer once detection completes + if (logBuffer.length >= jsonColorizeSampleSize) { + logBuffer = []; + } } return jsonDetected; };
🧹 Nitpick comments (1)
packages/webview/src/component/pods/PodLogs.svelte (1)
87-105: Consider adding a clarifying comment.The logic for determining
isJsonFormatcorrectly handles user preferences and auto-detection, but could benefit from a brief comment explaining the precedence.let lines: string[] = data.split('\n'); - //format json if the user asked for that or if they didn't set a preference and we auto-detected json logs + // Apply JSON colorization if: user selected FULL, or Auto mode detected JSON let isJsonFormat = colorfulOutputType === ColorOutputType.FULL || (!colorfulOutputType && isJsonDetected(lines));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/webview/src/component/pods/PodLogs.svelte(3 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 / windows-2022
- GitHub Check: Build extension OCI image
- GitHub Check: Build / macos-14
- GitHub Check: Build / ubuntu-24.04
🔇 Additional comments (7)
packages/webview/src/component/pods/PodLogs.svelte (7)
45-50: LGTM!The resize trigger correctly waits for the DOM update before dispatching the resize event, ensuring the terminal recalculates its size when logs appear.
56-66: LGTM!Defensive handling for padding and missing color mappings ensures the helper works correctly even in edge cases.
113-121: LGTM!The calculation is straightforward and correct for aligning multi-container log prefixes.
123-132: LGTM!The modulo operator correctly cycles through colors when there are more containers than available colors.
142-179: LGTM!The subscription logic correctly handles multi-container pods with
Promise.allSettled, ensures failed subscriptions don't block others, and properly tracks disposables for cleanup.
181-197: LGTM!Lifecycle hooks correctly initialize logs on mount, clean up event listeners, and dispose of subscriptions on destroy.
200-246: LGTM!The UI correctly implements the settings menu with color mode options, conditional rendering based on log availability, and proper accessibility attributes.
| let jsonDetected = $state(false); | ||
| let settingsMenuOpen = $state(false); | ||
| let logsTerminal = $state<Terminal>(); | ||
| let logBuffer: string[] = []; |
There was a problem hiding this comment.
Reset JSON detection state when viewing different pods.
jsonDetected and logBuffer are module-level variables that persist across different pods. When switching from one pod to another (e.g., viewing Pod A with JSON logs, then Pod B with plain logs), the cached detection result from Pod A incorrectly applies to Pod B.
Wrap these in a reactive statement that resets when object changes:
-let jsonDetected = $state(false);
+let jsonDetected = $state<boolean>(false);
let settingsMenuOpen = $state(false);
let logsTerminal = $state<Terminal>();
-let logBuffer: string[] = [];
+let logBuffer = $state<string[]>([]);
let disposables: IDisposable[] = [];
let settingsMenuRef: HTMLDivElement | undefined;
+
+// Reset detection state when viewing a different pod
+$effect(() => {
+ // Access object to create dependency
+ void object.metadata?.uid;
+ logBuffer = [];
+ jsonDetected = false;
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let jsonDetected = $state(false); | |
| let settingsMenuOpen = $state(false); | |
| let logsTerminal = $state<Terminal>(); | |
| let logBuffer: string[] = []; | |
| let jsonDetected = $state<boolean>(false); | |
| let settingsMenuOpen = $state(false); | |
| let logsTerminal = $state<Terminal>(); | |
| let logBuffer = $state<string[]>([]); | |
| let disposables: IDisposable[] = []; | |
| let settingsMenuRef: HTMLDivElement | undefined; | |
| // Reset detection state when viewing a different pod | |
| $effect(() => { | |
| // Access object to create dependency | |
| void object.metadata?.uid; | |
| logBuffer = []; | |
| jsonDetected = false; | |
| }); |
🤖 Prompt for AI Agents
In packages/webview/src/component/pods/PodLogs.svelte around lines 31–35,
jsonDetected and logBuffer are module-level and retain state across pod
switches; add a Svelte reactive statement that runs whenever the viewed pod
object changes and resets jsonDetected to its initial false state and clears
logBuffer (preserving existing settingsMenuOpen/logsTerminal behavior and
types). Ensure the reactive block only triggers on the pod-identifying variable
(e.g., object or its id) so each pod view starts with fresh JSON detection and
an empty buffer.
@MariaLeonova more actions in the horizontal bar are expected to come in follow-up PRs (see #417 (comment)). But the question is well grounded, I also wonder if we want to have this practically-empty bar for the moment, or if we want to implement a few actionable features without this bar, and having them configurable through other levers (I have made the proposal #542 for this second choice), and when we have enough configurable features, we can make them configurable locally. |
@feloy ok! Do you expect these changes to apply only in kubernetes section or globally across the app? |
This is open to discussion. These PRs are only for Kubernetes Pods logs, but they can be ported to the Podman containers logs if we want |
|
@adameska The PR introduces many changes (at least code refactoring, tests refactoring, toolbar, json coloring). Can you please list in the description of the PR the exhaustive list of changes, so I can help split this PR into several ones, and someone (you, me or someone else), can make small PRs on targeted changes prior to this one, to simplify the reviews |
Will do, i have no problem with you guys taking over these PR's and making them behave/function ideally to you guys. I was just missing some features we had in another application. Once this/these changes get merged i can update the other PR and do the same thing there so you guys can split out/merge as you see fit. |
5 participants
Logs before:
Logs after:
Features implemented: