Repository navigation
Fix Logs light mode controls - #3760
diegosouzapw merged 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds persistence for RequestLoggerV2’s refresh interval (via localStorage) and tweaks UI colors to improve readability in light mode, with accompanying unit checks.
Changes:
- Persist refresh interval setting in
RequestLoggerV2usinglocalStorageand clamped bounds. - Update hover/background and “clean history” button colors for better light-mode contrast.
- Add/adjust unit tests to assert these behaviors and some formatting assertions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/unit/request-logger-endpoints.test.ts | Adds string/regex-based unit checks for refresh-interval persistence and color-class changes. |
| src/shared/components/RequestLoggerV2.tsx | Implements persisted refresh interval + clamping; adjusts hover styling and some formatting. |
| src/app/(dashboard)/dashboard/logs/page.tsx | Updates “clean history” button text/hover colors for light/dark modes. |
| test("RequestLoggerV2 persists the custom refresh interval in localStorage", () => { | ||
| const content = fs.readFileSync("src/shared/components/RequestLoggerV2.tsx", "utf8"); | ||
| assert.match(content, /REFRESH_INTERVAL_STORAGE_KEY\s*=\s*"loggerRefreshIntervalSec"/); | ||
| assert.match(content, /readSavedRefreshIntervalSec/); | ||
| assert.match(content, /window\.localStorage\.setItem\(REFRESH_INTERVAL_STORAGE_KEY/); | ||
| assert.doesNotMatch( | ||
| content, | ||
| /const \[refreshIntervalSec, setRefreshIntervalSec\] = useState\(10\)/ | ||
| ); | ||
| }); | ||
|
|
||
| test("request log hover and clean-history colors stay readable in light mode", () => { | ||
| const requestLogger = fs.readFileSync("src/shared/components/RequestLoggerV2.tsx", "utf8"); | ||
| const logsPage = fs.readFileSync("src/app/(dashboard)/dashboard/logs/page.tsx", "utf8"); | ||
|
|
||
| assert.match(requestLogger, /hover:bg-sky-500\/10/); | ||
| assert.doesNotMatch(requestLogger, /cursor-pointer hover:bg-primary\/5/); | ||
| assert.match(logsPage, /text-red-700/); | ||
| assert.match(logsPage, /dark:text-red-300/); | ||
| assert.doesNotMatch(logsPage, /text-red-200/); | ||
| }); |
| @@ -131,7 +151,7 @@ const RequestLoggerV2 = forwardRef<RequestLoggerV2Handle, { initialSelectedId?: | |||
| const [detailLoggingLoading, setDetailLoggingLoading] = useState(false); | |||
| const [limit, setLimit] = useState(PAGE_SIZE); | |||
| const [hasMore, setHasMore] = useState(false); | |||
| const [refreshIntervalSec, setRefreshIntervalSec] = useState(10); | |||
| const [refreshIntervalSec, setRefreshIntervalSec] = useState(readSavedRefreshIntervalSec); | |||
| try { | ||
| window.localStorage.setItem(REFRESH_INTERVAL_STORAGE_KEY, String(next)); | ||
| } catch {} | ||
| return next; |
There was a problem hiding this comment.
Code Review
This pull request introduces persistence for the custom auto-refresh interval in localStorage within RequestLoggerV2.tsx, along with UI readability improvements for log table row hover states and the 'Clean History' button in light/dark modes. Unit tests were also added to verify these changes. The review feedback highlights two important issues: a potential SSR hydration mismatch caused by reading localStorage during initial state initialization, and an impure state updater containing side effects (localStorage.setItem). Both issues should be addressed to ensure robust SSR compatibility and clean React state management.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| @@ -131,7 +151,7 @@ const RequestLoggerV2 = forwardRef<RequestLoggerV2Handle, { initialSelectedId?: | |||
| const [detailLoggingLoading, setDetailLoggingLoading] = useState(false); | |||
| const [limit, setLimit] = useState(PAGE_SIZE); | |||
| const [hasMore, setHasMore] = useState(false); | |||
| const [refreshIntervalSec, setRefreshIntervalSec] = useState(10); | |||
| const [refreshIntervalSec, setRefreshIntervalSec] = useState(readSavedRefreshIntervalSec); | |||
There was a problem hiding this comment.
Hydration Mismatch in SSR
Initializing state directly from localStorage during the initial render (via readSavedRefreshIntervalSec) will cause a hydration mismatch in Next.js/SSR environments. This happens because the server renders the default value (10), while the client might initialize to a different saved value (e.g., 17), leading to mismatched HTML markup and console warnings.
To prevent this, initialize the state to DEFAULT_REFRESH_INTERVAL_SEC and load the saved value from localStorage in a useEffect on mount. Additionally, we can keep a ref of the current value to allow updateRefreshIntervalSec to remain dependency-free and perform the localStorage write outside of the state updater.
const [refreshIntervalSec, setRefreshIntervalSec] = useState(DEFAULT_REFRESH_INTERVAL_SEC);
// Load saved interval on mount to avoid SSR hydration mismatch
useEffect(() => {
setRefreshIntervalSec(readSavedRefreshIntervalSec());
}, []);
// Keep a ref of the current value to avoid dependency recreation in useCallback
const refreshIntervalSecRef = useRef(refreshIntervalSec);
useEffect(() => {
refreshIntervalSecRef.current = refreshIntervalSec;
}, [refreshIntervalSec]);
| const updateRefreshIntervalSec = useCallback( | ||
| (valueOrUpdater: number | ((current: number) => number)) => { | ||
| setRefreshIntervalSec((current) => { | ||
| const rawValue = | ||
| typeof valueOrUpdater === "function" ? valueOrUpdater(current) : valueOrUpdater; | ||
| const next = clampRefreshIntervalSec(rawValue); | ||
| try { | ||
| window.localStorage.setItem(REFRESH_INTERVAL_STORAGE_KEY, String(next)); | ||
| } catch {} | ||
| return next; | ||
| }); | ||
| }, | ||
| [] | ||
| ); |
There was a problem hiding this comment.
Impure State Updater (Side Effects in useState Setter)
Performing side effects like window.localStorage.setItem inside the setRefreshIntervalSec state updater function is an anti-pattern. React state updaters are expected to be pure functions and can be executed multiple times (e.g., in Strict Mode or Concurrent Mode).
By using the refreshIntervalSecRef set up on mount, we can read the current value, compute the next value, write it to localStorage, and update the state cleanly without any side effects inside the state updater.
const updateRefreshIntervalSec = useCallback(
(valueOrUpdater: number | ((current: number) => number)) => {
const current = refreshIntervalSecRef.current;
const rawValue =
typeof valueOrUpdater === "function" ? valueOrUpdater(current) : valueOrUpdater;
const next = clampRefreshIntervalSec(rawValue);
try {
window.localStorage.setItem(REFRESH_INTERVAL_STORAGE_KEY, String(next));
} catch {}
setRefreshIntervalSec(next);
},
[]
);
|
Thanks @rdself! The light-mode contrast fixes plus persisting the auto-refresh interval (clamped 1-300s, writes kept out of the React state updaters so StrictMode doesn't double-fire) are solid. The |
da58330
into
cheaper-inference:release/v3.8.24
| } | ||
|
|
||
| export function readSavedRefreshIntervalSec( | ||
| storage: RefreshIntervalStorage | null = getBrowserStorage() |
There was a problem hiding this comment.
WARNING: Storage access can throw before the try/catch
getBrowserStorage() is evaluated as a default parameter before either function body enters, so a blocked window.localStorage getter can bypass the intended graceful fallback. Move getBrowserStorage() into the try block (or make the helper catch and return null) so private/locked-down storage still degrades safely.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Reviewed by nex-n2-pro:free · 2,501,306 tokens |
… v3.8.24 merges These 5 files exceeded the frozen baseline solely because of already-merged, already-reviewed v3.8.24 PRs that landed after this branch froze its baseline: - src/lib/providers/validation.ts 4209→4302 (#3767 qwen-web fix) - src/shared/components/RequestLoggerV2.tsx 1232→1276 (#3760 logs light-mode) - open-sse/executors/antigravity.ts 1553→1572 (#3766 antigravity refresh) - open-sse/executors/base.ts 1199→1205 (#3756 xhigh effort) - open-sse/translator/response/openai-responses.ts 873→878 (#3762 tool-call integrity) None are touched by this branch; the bump reflects current release reality.
…contributor credits - Restructure [3.8.24] into ✨ Features / 🔒 Security / 🐛 Fixed / 📝 Maintenance - Add bullets for every PR landed since v3.8.23 that was missing: marketplace (#3656), strict-mode CC defaults (#3776), emergency-fallback flag (#3752), xhigh effort (#3756), Codex memory WS (#3749), IPv6 egress (#3777), marketplace SSRF (#3774), CodeQL/Dependabot (#3778), anthropic sampling (#3780), thinking passthrough (#3775), mcp dist entry (#3765), streamed tool args (#3762), logs light-mode (#3760), clean-history purge (#3751), quality-gates (#3757), docs gaps (#3453), file-size re-baseline (#3770), E415 publish guard, i18n prune - Move misplaced #3775 bullet out of [Unreleased] into [3.8.24] - Date [3.8.23] header (TBD -> 2026-06-12, the release tag date)
Integrated into release/v3.8.24
…contributor credits - Restructure [3.8.24] into ✨ Features / 🔒 Security / 🐛 Fixed / 📝 Maintenance - Add bullets for every PR landed since v3.8.23 that was missing: marketplace (cheaper-inference#3656), strict-mode CC defaults (cheaper-inference#3776), emergency-fallback flag (cheaper-inference#3752), xhigh effort (cheaper-inference#3756), Codex memory WS (cheaper-inference#3749), IPv6 egress (cheaper-inference#3777), marketplace SSRF (cheaper-inference#3774), CodeQL/Dependabot (cheaper-inference#3778), anthropic sampling (cheaper-inference#3780), thinking passthrough (cheaper-inference#3775), mcp dist entry (cheaper-inference#3765), streamed tool args (cheaper-inference#3762), logs light-mode (cheaper-inference#3760), clean-history purge (cheaper-inference#3751), quality-gates (cheaper-inference#3757), docs gaps (cheaper-inference#3453), file-size re-baseline (cheaper-inference#3770), E415 publish guard, i18n prune - Move misplaced cheaper-inference#3775 bullet out of [Unreleased] into [3.8.24] - Date [3.8.23] header (TBD -> 2026-06-12, the release tag date)
Summary
Validation
node --import tsx/esm --test tests/unit/request-logger-endpoints.test.ts tests/unit/request-logger-preferences.test.tsnpm run typecheck:corenpx eslint src/app/(dashboard)/dashboard/logs/page.tsx src/app/(dashboard)/dashboard/settings/components/FeatureFlagsGrid.tsx src/app/(dashboard)/dashboard/settings/components/FeatureFlagCard.tsx src/shared/components/RequestLoggerV2.tsx src/shared/components/requestLoggerPreferences.ts tests/unit/request-logger-endpoints.test.ts tests/unit/request-logger-preferences.test.ts(0 errors; existingRequestLoggerV2hook dependency warnings remain)oklab(... / 0.05)to cool-blueoklab(... / 0.1)on hover. Screenshots were reviewed at 1280x800.TLS Fingerprintto verify the restart banner, DB source badge, Reset All control, and summary count; desktop and 390px mobile screenshots were reviewed, with no horizontal overflow.