Repository navigation
fix(logs): default log viewer palette to the app theme - #1871
Conversation
The log viewer always opened dark, and in dark app theme the Sun/Moon toggle was hidden because the theme was passed as forceDark. Add a defaultDark prop that seeds the palette when the user hasn't picked one, follows theme changes until they do, and keeps the toggle visible. Fixes skyhook-io#1790
PR Summary by QodoDefault log viewer palette to the app theme
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Hub builds reject new prop
|
| setIsDark(forceDark) | ||
| } | ||
| }, [forceDark]) | ||
| // Follow host theme changes until the user picks a palette explicitly. |
There was a problem hiding this comment.
2. Theme comment repeats nearby logic 📘 Rule violation ⚙ Maintainability
Rule: Rule 3036542: Avoid explanatory comments that restate obvious code behavior
The comment above the second useEffect states that it follows host-theme changes until a saved palette exists, exactly restating the dependency list and local-storage guard. Removing it leaves the conditions clear from the effect itself and avoids requiring later changes to update both the implementation and a prose copy of its behavior.
Agent Prompt
## Issue description
The comment above the theme synchronization effect merely restates the behavior implemented by the effect and its local-storage guard.
## Fix Focus Areas
- packages/k8s-ui/src/components/logs/LogCore.tsx[184-190]
## Recommended Fix
Remove the redundant comment at line 184 while leaving the effect unchanged.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| createStream={makeStream} | ||
| overrideDownload={desktopDownload} | ||
| forceDark={theme === 'dark' ? true : undefined} | ||
| defaultDark={theme === 'dark'} |
There was a problem hiding this comment.
1. Hub builds reject new prop 🔗 Cross-repo conflict ≡ Correctness
@skyhook-io/radar-app now passes defaultDark to the shared log viewers, but its Kubernetes UI peer dependency still permits versions from 1.14.8 that predate this prop. Radar Hub Web resolves the two packages independently and currently pins Kubernetes UI 1.14.9, so upgrading Radar App alone can fail TypeScript validation or silently retain the old log theme behavior.
Agent Prompt
## Issue description
Radar App now requires a Kubernetes UI version whose log viewer props include `defaultDark`, but its peer dependency still advertises compatibility with older versions. Radar Hub Web installs Radar App and Kubernetes UI independently, so an app-only upgrade can select an incompatible combination.
## Fix Focus Areas
- web/package.json[37-43]
- web/src/components/logs/LogsViewer.tsx[41-47]
- web/src/components/logs/WorkloadLogsViewer.tsx[40-46]
## Recommended Fix
Raise the `@skyhook-io/k8s-ui` peer dependency floor in Radar App to the first published version containing `defaultDark`. Coordinate the Radar Hub Web dependency and lockfile update so Radar App and Kubernetes UI are upgraded together.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Light logs on a dark app are never wanted, so dark theme pins the palette and hides the toggle. Light theme starts light and keeps the toggle, so dark logs on a light app stay one click away. When a pin is lifted the viewer now re-reads the saved choice instead of keeping the pinned palette, and ignores saved values that aren't a palette choice.
nadaverell
left a comment
There was a problem hiding this comment.
Thanks, this fixes #1790. I pushed one change on top: in dark theme the logs stay dark and the toggle stays hidden (light logs on a dark app aren't something we want to offer); light theme gets your behaviour, starting light with the toggle available. I also made the viewer go back to the saved choice when switching from dark to light theme, with a test. Merging once CI is green.
Description
The logs panel always opened dark, and in dark app theme the web app passed
forceDark, which also hid the Sun/Moon toggle (as described in #1790).This adds a
defaultDarkprop toLogCore/LogsViewer/WorkloadLogsViewer: the palette the viewer starts with when the user hasn't picked one. UnlikeforceDark, it leaves the toggle visible.In the web app:
radar-logs-darkand wins from then on.forceDark, as before). Light logs on a dark app are never what anyone wants, so dark theme doesn't offer them.forceDarkworks the same for other consumers of@skyhook-io/k8s-ui; with no hint the default stays dark.Maintainer update: dark theme keeps logs pinned dark (product call), and the viewer re-reads the saved choice when that pin lifts.
Type of change
How has this been tested?
LogCore.theme.test.tsx: default, followsdefaultDark, saved choice wins,forceDarkhides toggle). The "follows defaultDark" test fails without the fix.packages/k8s-ui:npm testpasses (3889).tscshows the same 17 existing errors asmain, all in unrelated test files, and none inlogs/.web:npm run tsc,npm run lint(0 errors) andnpm test(1510) pass.Related issues
Fixes #1790
Note
Low Risk
UI-only theme behavior with backward-compatible props;
forceDarksemantics unchanged for other consumers.Overview
Fixes log viewer palette always opening dark and the web app using
forceDarkin dark theme, which hid the Sun/Moon toggle (#1790).Adds a
defaultDarkprop onLogCore(and passes it throughLogsViewer/WorkloadLogsViewer) for the initial palette when there is no savedradar-logs-darkchoice—unlikeforceDark, the toggle stays visible. Resolution order is unchanged for overrides:forceDark→ localStorage →defaultDark(still defaults to dark). A new effect updates the viewer whendefaultDarkchanges until the user toggles explicitly.The web wrappers now pass
defaultDark={theme === 'dark'}instead offorceDark, so logs match the app theme and follow theme switches while preserving per-user toggle preference.LogCore.theme.test.tsxcovers defaults,defaultDark, localStorage precedence, andforceDarkhiding the toggle.Reviewed by Cursor Bugbot for commit 7fbd039. Bugbot is set up for automated code reviews on this repo. Configure here.