YPE-1073: ensure bible reader loads with user settings from localStorage #134
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
🦋 Changeset detectedLatest commit: 5c8dda8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Greptile OverviewGreptile SummaryRefactored bible reader font preference loading to use Key Changes:
Issues Found:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant BibleReaderRoot
participant LocalStorage
participant State
participant DOM
User->>BibleReaderRoot: Mount component with props
BibleReaderRoot->>State: Initialize with validated fontSize & fontFamily from props
BibleReaderRoot->>DOM: First render with prop values
Note over DOM: User briefly sees default values
BibleReaderRoot->>LocalStorage: useLayoutEffect reads saved preferences
LocalStorage-->>BibleReaderRoot: Return saved fontSize (18) & fontFamily (Source Serif)
BibleReaderRoot->>State: setCurrentFontSize(18) & setCurrentFontFamily(Source Serif)
BibleReaderRoot->>DOM: Re-render with localStorage values
Note over DOM: User sees saved preferences (minimal flash)
User->>BibleReaderRoot: Change font size or family
BibleReaderRoot->>State: Update state
State->>LocalStorage: useEffect saves new value
BibleReaderRoot->>DOM: Render with new values
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| useLayoutEffect(() => { | ||
| const savedFontSize = localStorage.getItem('youversion-platform:reader:font-size'); | ||
| if (savedFontSize) { | ||
| const parsed = parseInt(savedFontSize); | ||
| if (parsed >= MIN_FONT_SIZE && parsed <= MAX_FONT_SIZE) { | ||
| setCurrentFontSize(parsed); | ||
| } | ||
| } | ||
|
|
||
| const savedFontFamily = localStorage.getItem('youversion-platform:reader:font-family'); | ||
| if (savedFontFamily) { | ||
| setCurrentFontFamily(savedFontFamily); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Missing SSR safety check - localStorage accessed without window check
| useLayoutEffect(() => { | |
| const savedFontSize = localStorage.getItem('youversion-platform:reader:font-size'); | |
| if (savedFontSize) { | |
| const parsed = parseInt(savedFontSize); | |
| if (parsed >= MIN_FONT_SIZE && parsed <= MAX_FONT_SIZE) { | |
| setCurrentFontSize(parsed); | |
| } | |
| } | |
| const savedFontFamily = localStorage.getItem('youversion-platform:reader:font-family'); | |
| if (savedFontFamily) { | |
| setCurrentFontFamily(savedFontFamily); | |
| } | |
| }, []); | |
| // Load saved preferences from localStorage before paint (avoids flash of default values) | |
| useLayoutEffect(() => { | |
| if (typeof window === 'undefined') return; | |
| const savedFontSize = localStorage.getItem('youversion-platform:reader:font-size'); | |
| if (savedFontSize) { | |
| const parsed = parseInt(savedFontSize); | |
| if (parsed >= MIN_FONT_SIZE && parsed <= MAX_FONT_SIZE) { | |
| setCurrentFontSize(parsed); | |
| } | |
| } | |
| const savedFontFamily = localStorage.getItem('youversion-platform:reader:font-family'); | |
| if (savedFontFamily) { | |
| setCurrentFontFamily(savedFontFamily); | |
| } | |
| }, []); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/components/bible-reader.tsx
Line: 114:127
Comment:
Missing SSR safety check - `localStorage` accessed without window check
```suggestion
// Load saved preferences from localStorage before paint (avoids flash of default values)
useLayoutEffect(() => {
if (typeof window === 'undefined') return;
const savedFontSize = localStorage.getItem('youversion-platform:reader:font-size');
if (savedFontSize) {
const parsed = parseInt(savedFontSize);
if (parsed >= MIN_FONT_SIZE && parsed <= MAX_FONT_SIZE) {
setCurrentFontSize(parsed);
}
}
const savedFontFamily = localStorage.getItem('youversion-platform:reader:font-family');
if (savedFontFamily) {
setCurrentFontFamily(savedFontFamily);
}
}, []);
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
All of the localStorage calls are within hooks which run on the client. This is why we don't need to check if window exists.
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
LGTM!
note: Sorry for the comment earlier that may have been confusing, but it has since been removed.
nitpick: I was a little weary removing the check for window, but since we are using a use client directive at the top, then it seems fine
All of the localStorage calls are within hooks which can only run on the client. |
No description provided.