fix: Update isPrettified handling on initial page load (fixes #356).
#366
Conversation
WalkthroughAppController now reads isPrettified from URL hash params using HASH_PARAM_NAMES on initial load and calls setIsPrettified(hashParams.isPrettified). logFileStore reads the view store's isPrettified and calls/awaits LogFileManager.setIsPrettified before loading page data. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant AppController
participant URLHashParser
participant ViewStore
participant LogFileStore
participant LogFileManager
Browser->>AppController: Load page
AppController->>URLHashParser: Parse hash params
URLHashParser-->>AppController: { isPrettified, ... }
AppController->>ViewStore: setIsPrettified(isPrettified)
AppController->>LogFileStore: trigger file load
LogFileStore->>ViewStore: read isPrettified
LogFileStore->>LogFileManager: await setIsPrettified(isPrettified)
LogFileStore->>LogFileStore: load page data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/components/AppController.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
src/components/AppController.tsx
🧠 Learnings (3)
📚 Learning: 2024-10-19T03:33:29.578Z
Learnt from: junhaoliao
PR: y-scope/yscope-log-viewer#94
File: new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx:99-118
Timestamp: 2024-10-19T03:33:29.578Z
Learning: In `new-log-viewer/src/components/CentralContainer/Sidebar/index.tsx`, when using `useEffect` to register window resize event handlers, it's acceptable to have an empty dependency array because the variables and functions used within the effect (`getPanelWidth`, `PANEL_CLIP_THRESHOLD_IN_PIXELS`, and `tabListRef`) are constants or refs declared outside the functional component and do not change.
Applied to files:
src/components/AppController.tsx
📚 Learning: 2025-06-01T13:40:12.222Z
Learnt from: zzxthehappiest
PR: y-scope/yscope-log-viewer#286
File: src/components/Editor/index.tsx:138-139
Timestamp: 2025-06-01T13:40:12.222Z
Learning: In the yscope-log-viewer codebase, when using Zustand stores in React components, the preferred pattern is to use `getState()` for static setters that never change (like `setLogEventNum`) to avoid unnecessary subscriptions, while using hooks for actions that do more than just setting values. All store state variables should be declared at the beginning of the component for consistency and clear dependency overview.
Applied to files:
src/components/AppController.tsx
📚 Learning: 2025-06-01T13:41:12.938Z
Learnt from: zzxthehappiest
PR: y-scope/yscope-log-viewer#286
File: src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.tsx:37-40
Timestamp: 2025-06-01T13:41:12.938Z
Learning: The `updateWindowUrlHashParams` function in `src/utils/url.ts` doesn't throw errors, so error handling is not needed when calling this function.
Applied to files:
src/components/AppController.tsx
🧬 Code Graph Analysis (1)
src/components/AppController.tsx (1)
src/typings/url.ts (1)
HASH_PARAM_NAMES(39-39)
🪛 GitHub Check: lint-check
src/components/AppController.tsx
[failure] 6-6:
There should be no space before '}'
[failure] 6-6:
There should be no space after '{'
[failure] 6-6:
Expected a line break after this opening brace
[failure] 6-6:
Imports must be broken into multiple lines if there are more than 1 elements
[failure] 1-1:
Expected a line break before this closing brace
[failure] 1-1:
There should be no space before '}'
[failure] 1-1:
There should be no space after '{'
[failure] 1-1:
Expected a line break after this opening brace
[failure] 1-1:
Run autofix to sort these imports!
[failure] 1-1:
Imports must be broken into multiple lines if there are more than 1 elements
🪛 GitHub Actions: lint
src/components/AppController.tsx
[error] 1-1: ESLint: Imports must be broken into multiple lines if there are more than 1 elements. (import-newlines/enforce)
🔇 Additional comments (2)
src/components/AppController.tsx (2)
57-57: Excellent fix for preserving isPrettified parameter from URL hash.This change correctly addresses issue #356 by using the actual
isPrettifiedvalue from the URL hash parameters instead of the default value. When users share URLs withisPrettified=true, this ensures the prettified state is preserved on initial page load.
54-62: LGTM! Clean and targeted fix that preserves URL state consistency.The implementation correctly addresses the PR objectives by ensuring
isPrettifiedparameter preservation on initial page load. The approach maintains both URL state (line 57) and application state (line 61) consistency, allowing users to share URLs with maintained prettified state.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/components/AppController.tsx (2)
63-71: Use a single, typed source of truth for isPrettified and add a fallback.Avoid mixing bracket-index and dot-prop access. Derive once from the hash using HASH_PARAM_NAMES and use that for both the URL update and the view store, with a safe default in case the param is absent.
const hashParams = getWindowUrlHashParams(); - updateWindowUrlHashParams({ - isPrettified: hashParams[HASH_PARAM_NAMES.IS_PRETTIFIED], - timestamp: URL_HASH_PARAMS_DEFAULT.timestamp, - }); - const {setIsPrettified} = useViewStore.getState(); - setIsPrettified(hashParams.isPrettified); + const isPrettifiedFromUrl = + hashParams[HASH_PARAM_NAMES.IS_PRETTIFIED] ?? URL_HASH_PARAMS_DEFAULT.isPrettified; + updateWindowUrlHashParams({ + isPrettified: isPrettifiedFromUrl, + timestamp: URL_HASH_PARAMS_DEFAULT.timestamp, + }); + const {setIsPrettified} = useViewStore.getState(); + setIsPrettified(isPrettifiedFromUrl);
57-61: StrictMode gotcha: early-return guard can drop the event listener in dev.With React 18 StrictMode, effects mount/cleanup twice in dev. Returning early with isInitialized.current can skip re-adding the handler after the intentional cleanup, leaving no listener. Consider removing this guard and relying on cleanup/idempotent code instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/components/AppController.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
src/components/AppController.tsx
🧠 Learnings (1)
📚 Learning: 2025-06-01T13:41:12.938Z
Learnt from: zzxthehappiest
PR: y-scope/yscope-log-viewer#286
File: src/components/CentralContainer/Sidebar/SidebarTabs/SearchTabPanel/QueryInputBox.tsx:37-40
Timestamp: 2025-06-01T13:41:12.938Z
Learning: The `updateWindowUrlHashParams` function in `src/utils/url.ts` doesn't throw errors, so error handling is not needed when calling this function.
Applied to files:
src/components/AppController.tsx
🧬 Code Graph Analysis (1)
src/components/AppController.tsx (1)
src/typings/url.ts (1)
HASH_PARAM_NAMES(39-39)
🔇 Additional comments (1)
src/components/AppController.tsx (1)
9-9: Import fix looks good (formatting and path).Using a single-element named import without the “.ts” extension aligns with the lint rules and TS conventions.
Good catch. Since we refactored loadPage without the isPrettified parameter, we should notify logFileManager to change isPrettified during the initial page load. |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/stores/logFileStore.ts (1)
119-127: Remove redundant state/URL updates (duplicate fileSrc set and FILE_PATH clearing)
fileSrcis set twice (Lines 120–124 and again on Line 141), andupdateWindowUrlSearchParamsis called twice with the same condition. Keep the first block and drop the second to reduce unnecessary state writes and URL manipulations.- set({fileSrc}); - if ("string" !== typeof fileSrc) { - updateWindowUrlSearchParams({[SEARCH_PARAM_NAMES.FILE_PATH]: null}); - }Also applies to: 141-144
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/stores/logFileStore.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
src/stores/logFileStore.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). (1)
- GitHub Check: build (macos-latest)
🔇 Additional comments (1)
src/stores/logFileStore.ts (1)
161-163: Correct sequencing: apply prettified state before loading page dataSetting/awaiting
setIsPrettifiedbeforeloadPage(cursor)ensures the initial page renders with the intended formatting. This directly addresses the regression in #356.
|
PR title: |
kirkrodrigues
changed the title
isPrettified handling on initial page load (fixes #356).isPrettified handling on initial page load (fixes #356).
Description
Fixes #356.
Checklist
breaking change.
Validation performed
Open this URL: http://localhost:3010/?filePath=https://yscope.s3.us-east-2.amazonaws.com/sample-logs/yarn-ubuntu-resourcemanager-ip-172-31-17-135.log.1.clp.zst#logEventNum=375558&isPrettified=true
Notice that the isPrettified=true parameter is kept.
Summary by CodeRabbit
Bug Fixes
Chores