refactor(web): dissolve domains/nudges/ into shared top-level directories (LUM-1919)#32204
Conversation
…ries (LUM-1919) Dissolves the nudges domain entirely — every file consumed by 2+ domains now lives in a top-level shared directory: - Platform detection (isIOSBrowser, isMacOSBrowser, useIsIOSWeb, useIsMacOSWeb) → utils/platform-detection.ts - Nudge Zustand store → stores/nudge-store.ts - GitHub/Discord/iOS/macOS nudge hooks + prefs → hooks/use-*-nudge.ts - localStorage helpers → utils/nudge-prefs.ts - Banner UI components → components/nudges/ Constants merged into their respective hook files to reduce file count; legacy-cleanup key strings inlined in the store to avoid circular deps. 7 cross-domain violations eliminated (95 → 88). Closes LUM-1919 Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Test Results — LUM-1919 Nudge Domain DissolutionMethod: Vite dev server (port 3001) + Playwright CDP route interception Results: All 3 tests passed
Console verification (chat page)All errors are expected 404s from unmocked |
There was a problem hiding this comment.
✦ APPROVE
Value: Dissolves a domain that was breaking its own convention — domains/nudges/ was consumed by 4 other domains, a violation of the "2+ domains → shared" rule. After this PR: platform detection lives in utils/, the Zustand store in stores/, the nudge hooks are self-contained, and every one of the 19 cross-domain allowlist entries for nudges is gone.
This is a pure refactor — Devin confirms no issues, no bot inline comments, and my read of the files agrees. Spot-checks:
platform-detection.ts ✅
isMacOSBrowser()callsisIOSBrowser()at the top of the function — iPadOS 13+ (which sends a macOS UA but has multitouch) is correctly excluded from the macOS nudge. The dependency is within the same file now, no import dance needed.useSyncExternalStore(noop, ...)is the right pattern for values that don't change after hydration — subscribe is a no-op because platform detection is stable for the session. Server snapshot returnsfalse(SSR safe).
nudge-store.ts ✅
createSelectors(useNudgeStoreBase)— canonical Zustand v5 selector pattern ✅vellum:nudge-prefsstarts withvellum→ cleared on logout bysession-cleanup.ts. Nudge state is correctly user-scoped.app.nudgeLegacy.cleanedstarts withapp.→ survives logout. Correct: if we already cleaned up stale keys, there's no reason to repeat it per-user.- Legacy key strings inlined directly — avoids circular dependency with the hook files, documented in a comment.
use-app-nudges.ts ✅
- Import paths point to
@/utils/platform-detection.js,@/hooks/use-ios-app-nudge.js, etc. — all match the new file locations. No staledomains/nudges/references. - Cascading visibility logic (platform → GitHub → Discord) is unchanged.
cross-domain-allowlist.json ✅
- All 19
nudgesentries removed. No other domain now has a cross-domain dependency onnudges.
utils/nudge-prefs.ts — one note (non-blocking): readBooleanPref/writeBooleanPref duplicate the logic just added by PR #32199 (getLocalBool/setLocalBool in local-settings.ts). These could be collapsed in a follow-up. The outer try/catch wrappers are also redundant since getLocalSetting/setLocalSetting already swallow exceptions internally — harmless but worth a cleanup pass once #32199 lands.
Vellum Constitution — Legibility: the codebase now says what it means. Code shared by 4 domains lives in shared directories, not inside one of them.
|
Re: |
Prompt / plan
Why needed: The
domains/nudges/directory was consumed by 4 other domains (onboarding, settings, chat, and internally by nudges itself). Per CONVENTIONS.md — "code used by 2+ domains moves to top-level shared directories." Rather than surgically extracting individual exports, the entire domain is dissolved because virtually every file had cross-domain consumers.What changed:
ios-app-platform.ts+mac-app-platform.tsutils/platform-detection.tsnudge-store.tsstores/nudge-store.tsnudge-prefs.tsutils/nudge-prefs.tsgithub-constants.ts+github-prefs.tshooks/use-github-nudge.tsdiscord-constants.ts+discord-prefs.tshooks/use-discord-nudge.tsios-app-constants.ts+ios-app-prefs.tshooks/use-ios-app-nudge.tsmac-app-constants.ts+mac-app-prefs.tshooks/use-macos-app-nudge.tscomponents/*.tsx(5 files)components/nudges/*.tsxWhat stayed the same: All behavior is preserved — no logic changes, just file moves and import path updates.
Alternatives not taken:
utils/: Considered keepinggithub-nudge-constants.ts,discord-nudge-constants.ts, etc. as separate files. Rejected because constants are small (2–6 per module), tightly coupled to their hook, and merging reduces file count from 17 → 12. The store's dependency on constants was broken by inlining the literal localStorage key strings in the legacy-cleanup section.Cross-domain violation accounting: 7 "nudges" entries removed from allowlist (95 → 88 violations remaining). 25 files changed, net -92 lines.
References:
Test plan
bunx tsc --noEmit— passes cleanbun run lint— passes cleanbun run audit:cross-domain— regenerated allowlist confirms 88 imports (down from 95)Closes LUM-1919
Link to Devin session: https://app.devin.ai/sessions/4b3b130bbf274e5487da3b4992883093
Requested by: @ashleeradka