-
Notifications
You must be signed in to change notification settings - Fork 963
feat(desktop): default new users to v2 and surface a v2 banner in v1 #4115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import { SidebarCard } from "@superset/ui/sidebar-card"; | ||
| import { AnimatePresence, motion } from "framer-motion"; | ||
| import { useIsV2CloudEnabled } from "renderer/hooks/useIsV2CloudEnabled"; | ||
| import { track } from "renderer/lib/analytics"; | ||
| import { useV2AvailableBannerStore } from "renderer/stores/v2-available-banner"; | ||
| import { useV2LocalOverrideStore } from "renderer/stores/v2-local-override"; | ||
|
|
||
| export function V2AvailableBanner() { | ||
| const isV2CloudEnabled = useIsV2CloudEnabled(); | ||
| const dismissed = useV2AvailableBannerStore((s) => s.dismissed); | ||
| const dismiss = useV2AvailableBannerStore((s) => s.dismiss); | ||
| const setOptInV2 = useV2LocalOverrideStore((s) => s.setOptInV2); | ||
|
|
||
| function handleSwitch() { | ||
| track("surface_toggled", { from: "v1", to: "v2", source: "v1_banner" }); | ||
| setOptInV2(true); | ||
| } | ||
|
|
||
| function handleDismiss() { | ||
| track("v2_banner_dismissed"); | ||
| dismiss(); | ||
| } | ||
|
|
||
| return ( | ||
| <AnimatePresence> | ||
| {!dismissed && ( | ||
| <motion.div | ||
| initial={{ opacity: 0, y: 8 }} | ||
| animate={{ opacity: 1, y: 0 }} | ||
| exit={{ opacity: 0, y: 8 }} | ||
| transition={{ duration: 0.2 }} | ||
| className="px-3 pb-2" | ||
| > | ||
| <SidebarCard | ||
| badge="New" | ||
| title="Superset v2 is here" | ||
| description="The new cloud workspace experience is now available." | ||
| actionLabel={isV2CloudEnabled ? undefined : "Switch to v2"} | ||
| onAction={isV2CloudEnabled ? undefined : handleSwitch} | ||
| onDismiss={handleDismiss} | ||
| /> | ||
| </motion.div> | ||
| )} | ||
| </AnimatePresence> | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export { V2AvailableBanner } from "./V2AvailableBanner"; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,29 +1,6 @@ | ||
| import { FEATURE_FLAGS } from "@superset/shared/constants"; | ||
| import { useFeatureFlagEnabled } from "posthog-js/react"; | ||
| import { useV2LocalOverrideStore } from "renderer/stores/v2-local-override"; | ||
|
|
||
| const IS_DEV = process.env.NODE_ENV === "development"; | ||
|
|
||
| /** | ||
| * Returns effective v2 state: remote PostHog flag AND local opt-in. | ||
| * Also returns the raw remote flag so the toggle can be shown conditionally. | ||
| */ | ||
| export function useIsV2CloudEnabled() { | ||
| const remoteV2Enabled = | ||
| useFeatureFlagEnabled(FEATURE_FLAGS.V2_CLOUD) ?? false; | ||
| const optInV2 = useV2LocalOverrideStore((s) => s.optInV2); | ||
|
|
||
| if (IS_DEV) { | ||
| return { | ||
| isV2CloudEnabled: optInV2, | ||
| isRemoteV2Enabled: true, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| /** The effective value — use this wherever you previously checked the flag directly. */ | ||
| isV2CloudEnabled: remoteV2Enabled && optInV2, | ||
| /** Whether the remote PostHog flag is on (for showing the toggle). */ | ||
| isRemoteV2Enabled: remoteV2Enabled, | ||
| }; | ||
| /** Returns whether v2 is currently active for this user. */ | ||
| export function useIsV2CloudEnabled(): boolean { | ||
| return useV2LocalOverrideStore((s) => s.optInV2); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| /** | ||
| * True when this install has been used before. Backed by `tabs-storage`, | ||
| * which is written the first time any workspace tab opens. Use this to | ||
| * distinguish a fresh install from a returning user. | ||
| */ | ||
| export function hasPriorSupersetUsage(): boolean { | ||
| if (typeof localStorage === "undefined") return false; | ||
| return localStorage.getItem("tabs-storage") !== null; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export { useV2AvailableBannerStore } from "./store"; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import { create } from "zustand"; | ||
| import { devtools, persist } from "zustand/middleware"; | ||
|
|
||
| interface V2AvailableBannerState { | ||
| dismissed: boolean; | ||
| dismiss: () => void; | ||
| } | ||
|
|
||
| export const useV2AvailableBannerStore = create<V2AvailableBannerState>()( | ||
| devtools( | ||
| persist( | ||
| (set) => ({ | ||
| dismissed: false, | ||
| dismiss: () => set({ dismissed: true }), | ||
| }), | ||
| { name: "v2-available-banner-v1" }, | ||
| ), | ||
| { name: "V2AvailableBannerStore" }, | ||
| ), | ||
| ); | ||
|
Comment on lines
+9
to
+20
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
A single Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/desktop/src/renderer/stores/v2-available-banner/store.ts
Line: 9-20
Comment:
**Shared dismissed flag hides banner across v1↔v2 transitions**
A single `dismissed` boolean is shared by both the v1 (`WorkspaceSidebar`) and v2 (`DashboardSidebar`) banner instances because they both read from the same `v2-available-banner-v1` store. Dismissing the announcement in one context silently suppresses it in the other. The most impactful path: a fresh-install user starts in v2, sees the announcement banner, dismisses it, then navigates to Settings → Experimental and toggles off v2 — the v1 "Switch to v2" banner will never appear because `dismissed` is already `true`. The user would have to go back to Settings to re-enable v2, with no in-sidebar nudge visible.
How can I resolve this? If you propose a fix, please make it concise. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,23 @@ | ||
| import { hasPriorSupersetUsage } from "renderer/lib/hasPriorSupersetUsage"; | ||
| import { create } from "zustand"; | ||
| import { devtools, persist } from "zustand/middleware"; | ||
|
|
||
| const IS_DEV = process.env.NODE_ENV === "development"; | ||
|
|
||
| interface V2LocalOverrideState { | ||
| /** When true, the user has opted into v2. v2 is gated behind both the remote flag and this opt-in. */ | ||
| optInV2: boolean; | ||
| setOptInV2: (optInV2: boolean) => void; | ||
| } | ||
|
|
||
| // Fresh installs default to v2; returning v1 users default to v1 and discover | ||
| // v2 via the in-sidebar banner. Persist hydration overrides this for anyone | ||
| // with a saved override. | ||
| const initialOptInV2 = !hasPriorSupersetUsage(); | ||
|
|
||
| export const useV2LocalOverrideStore = create<V2LocalOverrideState>()( | ||
| devtools( | ||
| persist( | ||
| (set) => ({ | ||
| optInV2: IS_DEV, | ||
| optInV2: initialOptInV2, | ||
|
Comment on lines
+11
to
+20
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Description: Find all callsites of setOptInV2 to see if fresh-install v2 flow explicitly persists true.
rg -n --type=ts --type=tsx -C3 'setOptInV2' 2>/dev/null || rg -n -C3 'setOptInV2'Repository: superset-sh/superset Length of output: 5762 🏁 Script executed: # Find the useMigrateV1DataToV2 hook to see if it calls setOptInV2
rg -n --type=ts --type=tsx "useMigrateV1DataToV2" -A 30 | head -100Repository: superset-sh/superset Length of output: 92 🏁 Script executed: # Search for auth layout files that might initialize v2 opt-in
fd -type f -name "*auth*" -o -name "*Auth*" | grep -E "\.(ts|tsx)$" | head -20Repository: superset-sh/superset Length of output: 235 🏁 Script executed: # Look for any place that calls setOptInV2 automatically (not in response to user action)
rg -n --type=ts --type=tsx "setOptInV2\(" -B 10 | grep -E "(useEffect|onMount|initialize)" -A 10Repository: superset-sh/superset Length of output: 92 🏁 Script executed: # Find the useMigrateV1DataToV2 hook implementation
rg -n --type ts "useMigrateV1DataToV2" -A 30 | head -150Repository: superset-sh/superset Length of output: 21601 🏁 Script executed: # Search for auth layout files
fd -type f \( -name "*auth*" -o -name "*Auth*" \) | grep -E "\.(ts|tsx)$" | head -20Repository: superset-sh/superset Length of output: 235 🏁 Script executed: # Look for setOptInV2 calls with surrounding context to find auto-initialization
rg -n "setOptInV2\(" -B 5 -A 2Repository: superset-sh/superset Length of output: 3040 🏁 Script executed: # Get the full useMigrateV1DataToV2 hook implementation
cat apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/useMigrateV1DataToV2.tsRepository: superset-sh/superset Length of output: 7532 🏁 Script executed: # Check the auth layout to see how useMigrateV1DataToV2 is used
cat apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsxRepository: superset-sh/superset Length of output: 5406 Fresh-install Zustand's
The only places The simplest fix is to force-persist the initial value immediately after store creation when no stored value is found: 🛡️ Proposed fix — force-persist initial value on first run export const useV2LocalOverrideStore = create<V2LocalOverrideState>()(
devtools(
persist(
(set) => ({
optInV2: initialOptInV2,
setOptInV2: (optInV2) => set({ optInV2 }),
}),
- { name: "v2-local-override-v2" },
+ {
+ name: "v2-local-override-v2",
+ onRehydrateStorage: () => (state) => {
+ // If no persisted value was found (fresh install), force a write
+ // so that initialOptInV2 survives across sessions independently
+ // of whether tabs-storage exists.
+ if (state && state.optInV2 === initialOptInV2) {
+ state.setOptInV2(initialOptInV2);
+ }
+ },
+ },
),
{ name: "V2LocalOverrideStore" },
),
);Alternatively, calling 🤖 Prompt for AI Agents |
||
| setOptInV2: (optInV2) => set({ optInV2 }), | ||
| }), | ||
| { name: "v2-local-override-v2" }, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasPriorSupersetUsagereturnsfalsewhenlocalStorageis unavailable, silently defaulting users to v2When
localStorageisundefined(e.g., in a test environment or a stripped Electron context), the function returnsfalse, causinginitialOptInV2to betrue. In a context wherelocalStoragetruly isn't available, the persist middleware will also fail to save, so the opt-in will be ephemeral — the user lands on v2 but their state isn't persisted, meaning the next load starts over at v2 again. Consider returningtrue(treat as "has prior usage" / stay conservative) when storage is inaccessible, to avoid silently opting users into v2 in degraded environments.Prompt To Fix With AI