fix: undefined state error #917
Conversation
colton-demetriou
requested review from
asanehisa,
benlife5,
briantstephan and
mkilpatrick
WalkthroughThe PR adds defensive checks and safer property access across InternalLayoutEditor, LayoutEditor, and ThemeEditor. It introduces a local Sequence Diagram(s)sequenceDiagram
autonumber
participant UI
participant Editor as InternalLayoutEditor
participant Histories
participant DevServer as Dev/Save Flow
Note over Editor,Histories: Load/derive current history
UI->>Editor: trigger save/dev
Editor->>Histories: const current = histories[index]
alt current or current.state missing
Editor-->>UI: return / abort (guard)
else current.state present
Editor->>DevServer: build payload using current.id and current.state.data/ui
DevServer-->>UI: success/error
end
sequenceDiagram
autonumber
participant UI
participant Editor as Layout/Theme Editor
participant LocalStorage as localHistories
UI->>Editor: load initial history
Editor->>LocalStorage: read localHistories
LocalStorage-->>Editor: array (may contain items without state)
Editor->>Editor: filter entries where h?.state
alt no valid histories
Editor-->>UI: do not assign puckInitialHistory (no early mutation)
else valid histories exist
Editor->>Editor: map to { id, state: migrate(data) }
Editor-->>UI: set puckInitialHistory and index
end
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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). (3)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/visual-editor/src/internal/components/ThemeEditor.tsx (1)
91-106: KeepInitialHistory.indexaligned with the filtered historiesHere you filter
localHistoriesdown to entries with astate, butlocalHistoryIndexis still computed from the unfilteredlocalHistoriesarray. If the last entry/entries are filtered out (e.g.,stateis null/undefined),localHistoryIndexcan be greater thanhistories.length - 1, so Puck’sinitialHistory.indexno longer corresponds to an element inhistoriesand may cause follow‑on undefined access elsewhere.You’re already computing the index from the filtered list in
LayoutEditor.tsx; I’d mirror that here:- const localHistoryIndex = localHistories.length - 1; - if (localHistoryIndex >= 0) { - devLogger.log("Theme Dev Mode - Using layout data from local storage"); - const histories = localHistories - .filter((h) => h?.state) - .map((h) => { - // strip ui state - return { id: h.id, state: { data: { ...h.state.data } } }; - }); + const histories = localHistories + .filter((h) => h?.state) + .map((h) => { + // strip ui state + return { id: h.id, state: { data: { ...h.state.data } } }; + }); + const localHistoryIndex = histories.length - 1; + if (localHistoryIndex >= 0) { + devLogger.log("Theme Dev Mode - Using layout data from local storage");[scratchpad_start]
[done]
ThemeEditor comment added.
[scratchpad_end] -->
🧹 Nitpick comments (3)
packages/visual-editor/src/internal/components/InternalLayoutEditor.tsx (1)
132-148: Early‑return when the selected history item has no stateOptional chaining here is good for avoiding the crash that Sentry was reporting, but if
histories[index]is missing or has nostate, we still enter this block and send a payload withhash/historyfields based onundefined.To keep downstream consumers from seeing half‑formed save‑state data, consider an explicit guard:
- if (layoutSaveState?.hash !== histories[index]?.id) { + const current = histories[index]; + if (!current?.state) { + return; + } + + if (layoutSaveState?.hash !== current.id) { if (templateMetadata.isDevMode && !templateMetadata.devOverride) { devLogger.logFunc("sendDevSaveStateData"); sendDevSaveStateData({ payload: { - devSaveStateData: JSON.stringify(histories[index]?.state?.data), + devSaveStateData: JSON.stringify(current.state.data), }, }); } else { devLogger.logFunc("saveLayoutSaveState"); saveLayoutSaveState({ payload: { - hash: histories[index]?.id, + hash: current.id, history: JSON.stringify({ - data: histories[index]?.state?.data, - ui: histories[index]?.state?.ui, + data: current.state.data, + ui: current.state.ui, }), }, }); } }packages/visual-editor/src/internal/components/LayoutEditor.tsx (2)
148-169: Treat “all filtered histories” as no valid dev localStorageThe new filtering + migration is a nice hardening, but there’s an edge case:
localHistoryArrayexists and parses, but every entry fails the!!history?.statefilter.localHistories.lengthbecomes0, solocalHistoryIndexis-1, we skip thesetPuckInitialHistoryblock but still setpuckInitialHistoryFetched(true)andreturn;.That leaves the editor with
puckInitialHistoryunset even though we do havelayoutDataavailable, effectively treating this as “empty layout” instead of “fall back to Content”.If the intent is to ignore corrupted/invalid localStorage and use Content instead, you can only early‑return when at least one valid history remains:
- const localHistories = ( - JSON.parse(localHistoryArray) as History<AppState>[] - ) - .filter((history): history is History<AppState> => !!history?.state) - .map((history) => ({ + const localHistories = ( + JSON.parse(localHistoryArray) as History<AppState>[] + ) + .filter((history): history is History<AppState> => !!history?.state) + .map((history) => ({ id: history.id, state: { data: migrate(history.state.data, migrationRegistry, puckConfig), ui: history.state.ui, }, })); const localHistoryIndex = localHistories.length - 1; - if (localHistoryIndex >= 0) { - setPuckInitialHistory({ - histories: localHistories as any, - index: localHistoryIndex, - appendData: false, - }); - } - setPuckInitialHistoryFetched(true); - return; + if (localHistoryIndex >= 0) { + setPuckInitialHistory({ + histories: localHistories as any, + index: localHistoryIndex, + appendData: false, + }); + setPuckInitialHistoryFetched(true); + return; + }This lets you naturally fall through to the “start fresh from Content” path when no valid histories remain.
255-262: Guard against sendingdevSaveStateDatawhen no history is availableOptional chaining here prevents the previous “cannot read property of undefined” crash, but when there’s no history (or no
state.dataon the last history),historyToSendbecomesundefinedand we still sendJSON.stringify(historyToSend).To avoid pushing an undefined payload to the parent, add a simple guard:
- const historyToSend = - puckInitialHistory?.histories?.[puckInitialHistory.histories.length - 1] - ?.state?.data; - - devLogger.logFunc("sendDevSaveStateData useEffect"); - sendDevLayoutSaveStateData({ - payload: { devSaveStateData: JSON.stringify(historyToSend) }, - }); + const historyToSend = + puckInitialHistory?.histories?.[puckInitialHistory.histories.length - 1] + ?.state?.data; + if (!historyToSend) { + return; + } + + devLogger.logFunc("sendDevSaveStateData useEffect"); + sendDevLayoutSaveStateData({ + payload: { devSaveStateData: JSON.stringify(historyToSend) }, + });This keeps the new null‑safety while avoiding unexpected “no‑data” payloads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/visual-editor/src/internal/components/InternalLayoutEditor.tsx(1 hunks)packages/visual-editor/src/internal/components/LayoutEditor.tsx(2 hunks)packages/visual-editor/src/internal/components/ThemeEditor.tsx(1 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). (2)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: semgrep/ci
colton-demetriou
changed the title
|
The robot nits look good to implement. I'm wondering when this occurs though. Is the history in a bad state or has it not loaded properly yet? |
Good question, I'm not entirely sure how to tell. I feel like the most likely culprit is where histories is initialized as [], so the the index looks for -1, and then .state would be undefined -- not sure how to verify though |
3 participants
Started locally and verified functionality still worked. This change just adds extra safety checks for potentially null states to hopefully prevent these sentries.
https://yext-engineering.sentry.io/issues/7028835198/?environment=prod&query=is%3Aunresolved&referrer=issue-stream&sort=freq