Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds v0.12.1 release notes to documentation. Introduces a shared Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/modals/modalsrenderer.tsx (1)
38-49: Upgrade onboarding effect misses updates when clientData arrives later.With an empty dependency array, the check may run before clientData/meta is ready and never re‑run. Add deps.
- useEffect(() => { + useEffect(() => { if (!globalPrimaryTabStartup) { return; } - if (!clientData.tosagreed) { + if (!clientData?.tosagreed) { return; } const lastVersion = clientData.meta?.["onboarding:lastversion"] ?? "v0.0.0"; if (semver.lt(lastVersion, CurrentOnboardingVersion)) { setUpgradeOnboardingOpen(true); } - }, []); + }, [clientData, globalPrimaryTabStartup, setUpgradeOnboardingOpen]);
🧹 Nitpick comments (9)
frontend/app/view/preview/preview-model.tsx (1)
511-516: Handle MDX consistently for icon and TOCYou render MDX as markdown, but icon and TOC checks still key on text/markdown only. Extend both to include text/mdx for a consistent UX.
Apply these diffs:
@@ - } else if (mimeType.startsWith("text/markdown")) { + } else if (mimeType.startsWith("text/markdown") || mimeType.startsWith("text/mdx")) { return "file-lines"; @@ - } else if (!isCeView && mimeType?.startsWith("text/markdown")) { + } else if (!isCeView && (mimeType?.startsWith("text/markdown") || mimeType?.startsWith("text/mdx"))) { return [frontend/app/onboarding/onboarding-upgrade.tsx (1)
13-21: Type-safety and duplication
- Avoid nullable lastVersion; initialize the ref with a default.
- De-duplicate hardcoded "v0.12.1" by using CurrentOnboardingVersion.
-import { useAtomValue } from "jotai"; +import { useAtomValue } from "jotai"; import { useEffect, useRef } from "react"; -import semver from "semver"; +import semver from "semver"; +import { CurrentOnboardingVersion } from "./onboarding-common"; @@ - const clientData = useAtomValue(atoms.client); - const initialVersionRef = useRef<string | null>(null); - - if (initialVersionRef.current == null) { - initialVersionRef.current = clientData.meta?.["onboarding:lastversion"] ?? "v0.0.0"; - } - - const lastVersion = initialVersionRef.current; + const clientData = useAtomValue(atoms.client); + const initialVersionRef = useRef<string>("v0.0.0"); + if (initialVersionRef.current === "v0.0.0") { + initialVersionRef.current = clientData.meta?.["onboarding:lastversion"] ?? "v0.0.0"; + } + const lastVersion: string = initialVersionRef.current; @@ - if (semver.gte(lastVersion, "v0.12.1")) { + if (semver.gte(lastVersion, CurrentOnboardingVersion)) { globalStore.set(modalsModel.upgradeOnboardingOpen, false); } }, [lastVersion]); @@ - if (semver.gte(lastVersion, "v0.12.1")) { + if (semver.gte(lastVersion, CurrentOnboardingVersion)) { return null; } @@ - if (semver.gte(lastVersion, "v0.12.0")) { + if (semver.gte(lastVersion, "v0.12.0")) { return <UpgradeOnboardingModal_v0_12_1 />; }frontend/app/onboarding/onboarding-upgrade-v0121.tsx (3)
36-43: Debounced resize: cancel on cleanupCall cancel on the debounced function to avoid stray timers/listeners.
- useEffect(() => { + useEffect(() => { updateModalHeight(); - const debouncedUpdateModalHeight = debounce(150, updateModalHeight); + const debouncedUpdateModalHeight = debounce(150, updateModalHeight); window.addEventListener("resize", debouncedUpdateModalHeight); return () => { window.removeEventListener("resize", debouncedUpdateModalHeight); + // throttle-debounce v5 exposes .cancel() + (debouncedUpdateModalHeight as any).cancel?.(); }; }, []);
22-34: Prefer CSS max-height over imperative heightYou can avoid manual height writes by using Tailwind max-h-[90vh] and rely on overflow auto.
- const updateModalHeight = () => { - const windowHeight = window.innerHeight; - setIsCompact(windowHeight < 800); - if (modalRef.current) { - const modalHeight = modalRef.current.offsetHeight; - const maxHeight = windowHeight * 0.9; - if (maxHeight < modalHeight) { - modalRef.current.style.height = `${maxHeight}px`; - } else { - modalRef.current.style.height = "auto"; - } - } - }; + const updateModalHeight = () => setIsCompact(window.innerHeight < 800); @@ - <FlexiModal className={`w-[600px] rounded-[10px] ${paddingClass} relative overflow-hidden`} ref={modalRef}> + <FlexiModal className={`w-[600px] max-h-[90vh] rounded-[10px] ${paddingClass} relative overflow-hidden`} ref={modalRef}>
52-62: Persist meta before closing (await RPC)Make handleClose async and await SetMetaCommand to reduce risk of dropping the update under fast unmounts.
- const handleClose = () => { + const handleClose = async () => { const clientId = globalStore.get(atoms.clientId); - RpcApi.SetMetaCommand(TabRpcClient, { + await RpcApi.SetMetaCommand(TabRpcClient, { oref: WOS.makeORef("client", clientId), meta: { "onboarding:lastversion": CurrentOnboardingVersion }, }); globalStore.set(modalsModel.upgradeOnboardingOpen, false); setTimeout(() => { globalRefocus(); }, 10); };frontend/app/onboarding/onboarding-upgrade-v0120.tsx (3)
38-45: Cancel the debounced handler on unmount to avoid stray invocations.Call cancel() on the debounced function in cleanup.
useEffect(() => { updateModalHeight(); - const debouncedUpdateModalHeight = debounce(150, updateModalHeight); + const debouncedUpdateModalHeight = debounce(150, updateModalHeight); window.addEventListener("resize", debouncedUpdateModalHeight); return () => { window.removeEventListener("resize", debouncedUpdateModalHeight); + // throttle-debounce provides cancel() + (debouncedUpdateModalHeight as any).cancel?.(); }; }, []);
22-23: Guard initial window access (SSR‑safe) or confirm this is client‑only.Directly reading window in initial state can break SSR. If SSR is possible, gate it.
- const [isCompact, setIsCompact] = useState<boolean>(window.innerHeight < 800); + const [isCompact, setIsCompact] = useState<boolean>(() => + typeof window !== "undefined" ? window.innerHeight < 800 : false + );
106-116: Duplicate setting of onboarding:lastversion — confirm intent.You set "onboarding:lastversion" here on completion and also inside OnboardingFeatures mount. This can mark the upgrade as seen even if the user never reaches completion. If that’s desired, ignore; otherwise, set only on completion.
frontend/app/modals/modalsrenderer.tsx (1)
50-52: Avoid depending on the ephemeral rtn array identity.Drive modalOpen from state booleans to reduce unnecessary effect runs.
- useEffect(() => { - globalStore.set(atoms.modalOpen, rtn.length > 0); - }, [rtn]); + useEffect(() => { + const anyOpen = newInstallOnboardingOpen || upgradeOnboardingOpen || modals.length > 0; + globalStore.set(atoms.modalOpen, anyOpen); + }, [newInstallOnboardingOpen, upgradeOnboardingOpen, modals]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/docs/releasenotes.mdx(1 hunks)frontend/app/modals/about.tsx(1 hunks)frontend/app/modals/modalsrenderer.tsx(1 hunks)frontend/app/onboarding/onboarding-common.tsx(1 hunks)frontend/app/onboarding/onboarding-features.tsx(1 hunks)frontend/app/onboarding/onboarding-upgrade-v0120.tsx(1 hunks)frontend/app/onboarding/onboarding-upgrade-v0121.tsx(1 hunks)frontend/app/onboarding/onboarding-upgrade.tsx(1 hunks)frontend/app/view/preview/preview-model.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/app/onboarding/onboarding-upgrade.tsx (5)
frontend/app/store/global.ts (2)
atoms(811-811)globalStore(834-834)version.cjs (1)
semver(31-31)frontend/app/store/modalmodel.ts (1)
modalsModel(45-45)frontend/app/onboarding/onboarding-upgrade-v0121.tsx (1)
UpgradeOnboardingModal_v0_12_1(170-170)frontend/app/onboarding/onboarding-upgrade-v0120.tsx (1)
UpgradeOnboardingModal_v0_12_0(201-201)
frontend/app/onboarding/onboarding-upgrade-v0121.tsx (7)
frontend/app/store/keymodel.ts (3)
disableGlobalKeybindings(653-653)enableGlobalKeybindings(654-654)globalRefocus(656-656)frontend/app/store/global.ts (2)
atoms(811-811)WOS(861-861)frontend/app/store/wshclientapi.ts (1)
RpcApi(542-542)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/app/onboarding/onboarding-common.tsx (1)
CurrentOnboardingVersion(4-4)frontend/app/store/modalmodel.ts (1)
modalsModel(45-45)frontend/app/modals/modal.tsx (1)
FlexiModal(112-112)
frontend/app/onboarding/onboarding-upgrade-v0120.tsx (2)
frontend/app/onboarding/onboarding-common.tsx (1)
CurrentOnboardingVersion(4-4)frontend/app/onboarding/onboarding-features.tsx (1)
OnboardingFeatures(305-364)
🪛 Biome (2.1.2)
frontend/app/onboarding/onboarding-upgrade-v0120.tsx
[error] 146-146: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
⏰ 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). (5)
- GitHub Check: Build Docsite
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: merge-gatekeeper
🔇 Additional comments (7)
frontend/app/modals/about.tsx (1)
20-20: Padding change: confirm modal chrome consistencyAdding top padding may shift header alignment versus other modals. Quick visual check across common modals recommended.
docs/docs/releasenotes.mdx (1)
9-28: Verify date and cross-links
- Confirm the release date “Oct 20, 2025” matches the actual tag publish date.
- Consider linking to docs for OSC 7 and AI feedback buttons for discoverability.
frontend/app/onboarding/onboarding-upgrade-v0120.tsx (3)
191-197: Verify FlexiModal forwards the ref to an HTMLElement.updateModalHeight relies on offsetHeight and style.height. If FlexiModal doesn’t forwardRef to a DOM node, this will no‑op.
68-69: Harden window.open against opener leaks.Use noopener/noreferrer (and clear opener) when opening GitHub in a new tab.
- window.open("https://github.com/wavetermdev/waveterm?ref=upgrade", "_blank"); + const newTab = window.open( + "https://github.com/wavetermdev/waveterm?ref=upgrade", + "_blank", + "noopener,noreferrer" + ); + if (newTab) newTab.opener = null;⛔ Skipped due to learnings
Learnt from: sawka PR: wavetermdev/waveterm#2433 File: frontend/app/onboarding/onboarding-upgrade.tsx:116-116 Timestamp: 2025-10-15T03:20:22.658Z Learning: In Electron applications, external links opened with `target="_blank"` open in the system's default browser, not within the Electron app context. This creates a security boundary that prevents `window.opener` access, so the typical web security concern about missing `rel="noopener noreferrer"` does not apply in the Electron context.
146-149: Add rel="noopener noreferrer" to external link opened in a new tab.Prevents reverse‑tabnabbing and stops the new page from accessing window.opener.
Apply this diff:
- <a target="_blank" href="https://discord.gg/XfvZ334gwU" className="hover:underline"> + <a target="_blank" rel="noopener noreferrer" href="https://discord.gg/XfvZ334gwU" className="hover:underline">⛔ Skipped due to learnings
Learnt from: sawka PR: wavetermdev/waveterm#2433 File: frontend/app/onboarding/onboarding-upgrade.tsx:116-116 Timestamp: 2025-10-15T03:20:22.658Z Learning: In Electron applications, external links opened with `target="_blank"` open in the system's default browser, not within the Electron app context. This creates a security boundary that prevents `window.opener` access, so the typical web security concern about missing `rel="noopener noreferrer"` does not apply in the Electron context.frontend/app/onboarding/onboarding-common.tsx (1)
4-4: Centralized version constant looks good.frontend/app/onboarding/onboarding-features.tsx (1)
15-15: Importing CurrentOnboardingVersion from the new common module is correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/app/onboarding/onboarding-upgrade.tsx (3)
6-10: Semver import fix looks good; also use centralized CurrentOnboardingVersion (avoid hard‑coding “v0.12.1”).Prevents drift on future releases and aligns with the PR’s centralization goal.
import { modalsModel } from "@/app/store/modalmodel"; import { useAtomValue } from "jotai"; import { useEffect, useRef } from "react"; import * as semver from "semver"; +import { CurrentOnboardingVersion } from "./onboarding-common"; import { UpgradeOnboardingModal_v0_12_0 } from "./onboarding-upgrade-v0120"; import { UpgradeOnboardingModal_v0_12_1 } from "./onboarding-upgrade-v0121";
22-26: Compare against a normalized target version; update effect deps.Avoid parsing differences (“v” prefix) and ensure the close action reacts if the target bumps.
-useEffect(() => { - if (semver.gte(lastVersion, "v0.12.1")) { - globalStore.set(modalsModel.upgradeOnboardingOpen, false); - } - }, [lastVersion]); +const targetVersion = semver.coerce(CurrentOnboardingVersion)?.version ?? "0.0.0"; +useEffect(() => { + if (semver.gte(lastVersion, targetVersion)) { + globalStore.set(modalsModel.upgradeOnboardingOpen, false); + } +}, [lastVersion, targetVersion]);If pre‑releases like
v0.12.1-rc.1should count as “seen 0.12.1”, pass{ includePrerelease: true }togte. Confirm desired behavior.
28-36: Normalize once, then branch; avoid brittle “v” prefix compares and duplicate parses.This reduces edge‑case failures and keeps logic ready for future version bumps.
- if (semver.gte(lastVersion, "v0.12.1")) { + if (semver.gte(lastVersion, targetVersion)) { return null; } - if (semver.gte(lastVersion, "v0.12.0")) { + // Boundary for showing the 0.12.1 upgrade modal if user already saw 0.12.0 + if (semver.gte(lastVersion, "0.12.0")) { return <UpgradeOnboardingModal_v0_12_1 />; } return <UpgradeOnboardingModal_v0_12_0 />;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/modals/modalsrenderer.tsx(1 hunks)frontend/app/onboarding/onboarding-upgrade.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/modals/modalsrenderer.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/app/onboarding/onboarding-upgrade.tsx (5)
frontend/app/store/global.ts (2)
atoms(811-811)globalStore(834-834)version.cjs (1)
semver(31-31)frontend/app/store/modalmodel.ts (1)
modalsModel(45-45)frontend/app/onboarding/onboarding-upgrade-v0121.tsx (1)
UpgradeOnboardingModal_v0_12_1(170-170)frontend/app/onboarding/onboarding-upgrade-v0120.tsx (1)
UpgradeOnboardingModal_v0_12_0(201-201)
⏰ 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). (5)
- GitHub Check: Build Docsite
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: merge-gatekeeper
| const clientData = useAtomValue(atoms.client); | ||
| const initialVersionRef = useRef<string | null>(null); | ||
|
|
||
| const updateModalHeight = () => { | ||
| const windowHeight = window.innerHeight; | ||
| setIsCompact(windowHeight < 800); | ||
| if (modalRef.current) { | ||
| const modalHeight = modalRef.current.offsetHeight; | ||
| const maxHeight = windowHeight * 0.9; | ||
| if (maxHeight < modalHeight) { | ||
| modalRef.current.style.height = `${maxHeight}px`; | ||
| } else { | ||
| modalRef.current.style.height = "auto"; | ||
| } | ||
| } | ||
| }; | ||
| if (initialVersionRef.current == null) { | ||
| initialVersionRef.current = clientData.meta?.["onboarding:lastversion"] ?? "v0.0.0"; | ||
| } | ||
|
|
||
| useEffect(() => { | ||
| updateModalHeight(); | ||
| const debouncedUpdateModalHeight = debounce(150, updateModalHeight); | ||
| window.addEventListener("resize", debouncedUpdateModalHeight); | ||
| return () => { | ||
| window.removeEventListener("resize", debouncedUpdateModalHeight); | ||
| }; | ||
| }, []); | ||
| const lastVersion = initialVersionRef.current; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Nullable ref + “freeze at mount” can misroute onboarding; normalize and set once when meta hydrates.
- TS: lastVersion is
string | nulland passed tosemver.gte. - UX: if
clientData.metaloads after first render, you permanently freeze to"v0.0.0"and may show the wrong modal. - Robustness: guard invalid versions via
semver.coerce.
- const clientData = useAtomValue(atoms.client);
- const initialVersionRef = useRef<string | null>(null);
+ const clientData = useAtomValue(atoms.client);
+ // Store a normalized semver string once (e.g., "0.12.1"); default to "0.0.0".
+ const initialVersionRef = useRef<string>("0.0.0");
- if (initialVersionRef.current == null) {
- initialVersionRef.current = clientData.meta?.["onboarding:lastversion"] ?? "v0.0.0";
- }
+ // Set exactly once when the real value becomes available.
+ useEffect(() => {
+ if (initialVersionRef.current === "0.0.0") {
+ const raw = clientData.meta?.["onboarding:lastversion"];
+ if (raw) {
+ initialVersionRef.current = semver.coerce(raw)?.version ?? "0.0.0";
+ }
+ }
+ }, [clientData.meta?.["onboarding:lastversion"]]);
- const lastVersion = initialVersionRef.current;
+ const lastVersion = initialVersionRef.current; // normalized stringDo we ever expect non‑semver values (e.g., empty string) in onboarding:lastversion? If yes, the above coerce path is required to prevent runtime errors.
I'll verify the review comment by investigating the codebase. Let me gather information about the actual usage, types, and context.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find all usages of lastVersion and semver in the onboarding-upgrade.tsx file
echo "=== Searching for lastVersion and semver usage in the file ==="
rg -n "lastVersion|semver\." frontend/app/onboarding/onboarding-upgrade.tsx -A 2 -B 2
Length of output: 526
🏁 Script executed:
#!/bin/bash
# Check the full context of the onboarding-upgrade.tsx file
echo "=== Full file content ==="
cat frontend/app/onboarding/onboarding-upgrade.tsxLength of output: 1335
🏁 Script executed:
#!/bin/bash
# Search for where atoms.client is defined
echo "=== Searching for atoms.client definition ==="
rg -n "atoms\.client|export.*client" --type=ts --type=tsx -C 3 | head -50Length of output: 73
🏁 Script executed:
#!/bin/bash
# Search for onboarding:lastversion assignments
echo "=== Searching for onboarding:lastversion usage ==="
rg -n "onboarding:lastversion" --type=ts --type=tsx -C 2Length of output: 78
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for atoms.client definition (without tsx type)
echo "=== Searching for atoms.client definition ==="
rg -n "atoms\.client|export.*client.*=" --type=ts -C 3 | head -60
Length of output: 4882
🏁 Script executed:
#!/bin/bash
# Search for onboarding:lastversion usage (without tsx type)
echo "=== Searching for onboarding:lastversion assignments ==="
rg -n "onboarding:lastversion" -C 3Length of output: 4831
🏁 Script executed:
#!/bin/bash
# Check if semver.coerce is used elsewhere or available
echo "=== Checking semver package usage ==="
rg -n "semver\.(coerce|parse|valid)" -C 2Length of output: 37
🏁 Script executed:
#!/bin/bash
# Check imports in package.json to understand semver version
echo "=== Checking semver dependency ==="
rg -n '"semver"' package.jsonLength of output: 66
---
🏁 Script executed:
```shell
#!/bin/bash
# Find CurrentOnboardingVersion constant
echo "=== Searching for CurrentOnboardingVersion definition ==="
rg -n "CurrentOnboardingVersion" -C 2
Length of output: 6292
🏁 Script executed:
#!/bin/bash
# Search for atoms.client to understand if meta is async/sync
echo "=== Searching for atoms.client or client atom definition ==="
rg -n "client.*atom|atom.*client|export.*client" --type=ts -C 3 | head -80Length of output: 5716
🏁 Script executed:
#!/bin/bash
# Check if there's any async loading or hydration logic for client meta
echo "=== Searching for meta loading or hydration patterns ==="
rg -n "meta|hydrate|clientData" frontend/app/store/global.ts -C 2 | head -60Length of output: 1878
---
🌐 Web query:
```web
semver.gte function handles version with and without v prefix
💡 Result:
Yes — the common semver libraries treat a leading "v" as optional. The npm/node-semver parser explicitly strips a leading "=" or "v" before comparing, so semver.gte('v1.2.3', '1.2.0') and semver.gte('1.2.3', 'v1.2.0') will work as expected. [1]
(Other implementations follow the same convention — they accept versions with or without the "v" prefix.) [2]
Sources:
- npm/node-semver README (strips leading "=" or "v"). [1]
- Example libraries/docs noting support for prefixed and non-prefixed versions. [2]
Type safety and timing: lastVersion freezes to default if meta hydrates late.
The component has two genuine issues:
-
Type mismatch:
lastVersionis typed asstring | nullbut passed tosemver.gte()which expectsstring. TypeScript will error. -
Freezing value: The initialization runs in the render body (lines 16–18), not in an effect. If
clientData.metaarrives after first render, the ref still gets set to the fallback"v0.0.0"on mount and never updates, causing the wrong modal to display.
The suggested fix resolves both by using useEffect to normalize the version exactly once when meta becomes available, and by using semver.coerce to handle version format consistently.
- const clientData = useAtomValue(atoms.client);
- const initialVersionRef = useRef<string | null>(null);
+ const clientData = useAtomValue(atoms.client);
+ // Store a normalized semver string once (e.g., "0.12.1"); default to "0.0.0".
+ const initialVersionRef = useRef<string>("0.0.0");
- if (initialVersionRef.current == null) {
- initialVersionRef.current = clientData.meta?.["onboarding:lastversion"] ?? "v0.0.0";
- }
+ // Set exactly once when the real value becomes available.
+ useEffect(() => {
+ if (initialVersionRef.current === "0.0.0") {
+ const raw = clientData.meta?.["onboarding:lastversion"];
+ if (raw) {
+ initialVersionRef.current = semver.coerce(raw)?.version ?? "0.0.0";
+ }
+ }
+ }, [clientData.meta?.["onboarding:lastversion"]]);
- const lastVersion = initialVersionRef.current;
+ const lastVersion = initialVersionRef.current; // normalized stringThere was a problem hiding this comment.
clientdata is loaded in wave.ts and is awaited so it is always available BEFORE any react runs. 2nd, we DONT want updates as we want the modal we render to not just randomly disappear in the middle of using it. once we pop it open we're committed.
There was a problem hiding this comment.
@sawka, thank you for the clarification! You're absolutely right—if clientData is pre-loaded and awaited in wave.ts before React initializes, then the timing concern is invalid. And freezing the version at mount to prevent the modal from disappearing mid-interaction is the correct design choice.
I appreciate you explaining the architecture. My review was based on incorrect assumptions about the data loading sequence.
✏️ Learnings added
Learnt from: sawka
PR: wavetermdev/waveterm#2465
File: frontend/app/onboarding/onboarding-upgrade.tsx:13-21
Timestamp: 2025-10-21T05:09:26.906Z
Learning: In the waveterm codebase, clientData is loaded and awaited in wave.ts before React runs, ensuring it is always available when components mount. This means atoms.client will have data on first render.
Learnt from: sawka
PR: wavetermdev/waveterm#2465
File: frontend/app/onboarding/onboarding-upgrade.tsx:13-21
Timestamp: 2025-10-21T05:09:26.906Z
Learning: The onboarding upgrade modal in frontend/app/onboarding/onboarding-upgrade.tsx intentionally freezes the lastVersion at mount using a ref to prevent the modal from changing or disappearing mid-interaction when the user is going through the onboarding flow.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
No description provided.