fix(web): delete vestigial AppRootProvider and fix theme selector mismatch#31324
Conversation
…to portal to document.body LUM-1711 AppRootProvider was carried from the platform repo but serves no purpose in the new stack — all theme CSS variables are defined on :root via tokens.css and applied to document.documentElement by applyThemePreference(). Zero CSS rules target .app-root. Portals to document.body inherit all theme tokens. The provider was never mounted, so 6 consumers that depended on useAppRootContainer() were silently broken (returning null, causing modals and popovers to not render). - Delete app-root-context.tsx (AppRootProvider + useAppRootContainer hook) - Update 6 consumers to portal to document.body instead - Remove dead if-null-return-null guards that prevented rendering 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:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| /> | ||
| </div>, | ||
| portalTarget, | ||
| document.body, |
There was a problem hiding this comment.
🚩 Future dark/velvet theme support requires data-theme on , not a scoped element
The CSS in tokens.css defines dark and velvet themes via [data-theme="dark"] and [data-theme="velvet"] selectors. Currently no data-theme attribute is set anywhere in the app (index.html has a bare <html lang="en">). When theme switching is implemented, the data-theme attribute must be placed on <html> (or at minimum an ancestor of <body>) for portaled content on document.body to inherit the correct theme tokens. If it were placed on an inner wrapper element (as .app-root would have been), portals to document.body would fall back to light-mode :root values. This is not a current bug but a constraint worth documenting.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is a pre-existing architectural detail, not introduced by this PR.
The constraint the bot identifies is already satisfied by the current setup: applyThemePreference() applies theme classes to document.documentElement (the <html> element), and document.body is a child of <html>, so portals to document.body correctly inherit all theme tokens via CSS inheritance regardless of whether theming uses data-theme attributes or CSS classes.
Worth noting: there's a pre-existing gap between tokens.css (which uses [data-theme="dark"] selectors) and applyThemePreference() (which toggles CSS classes like .dark), but that's unrelated to this PR's portal target change.
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
✦ APPROVE
What this does: Deletes AppRootProvider/useAppRootContainer (ported from the platform repo but never wired up here), removes the dead null guards that were silently preventing 6 components from rendering, and migrates all portal consumers to document.body.
This is more of a bug fix than a refactor — command-palette, trust-rules-modal, trust-rule-form-modal, avatar-management-modal, AttachmentPreviewModal, and context-window-indicator tooltip were all rendering null 100% of the time because AppRootProvider was never mounted in the route tree.
Deletion safety verified:
app-root-context.tsxgone at HEAD ✅tokens.csshas zero.app-rootoccurrences — all CSS custom properties (--surface-lift,--border-base, etc.) are defined on:root, so portals todocument.bodyinherit them via normal CSS inheritance ✅AppRootProviderwas never mounted inmain.tsx,root-layout.tsx, or the provider stack — the null guards were therefore always firing ✅- No remaining imports of
app-root-contextanywhere inapps/web/src/✅
The old ContextWindowIndicator comment ("portal into .app-root so appTheme.css tokens resolve") was describing the platform repo's scoping model, not this one. :root scoping in the Vite SPA means the comment was cargo-culted and the AppRootProvider was never necessary.
Devin finding:
Devin flagged that dark/velvet theme support requires data-theme on <html> for portals to document.body to inherit the correct tokens. Self-resolved: applyThemePreference() targets document.documentElement (the <html> element), so document.body is a child and inherits theme tokens regardless. ✅
Devin also noted a pre-existing gap: tokens.css uses [data-theme="dark"] selectors while applyThemePreference() toggles CSS class names like .dark. These two systems are misaligned — one of them doesn't apply. Worth a follow-up audit/ticket to reconcile which selector strategy is canonical and align the two. Not introduced by this PR.
Pattern check:
All 6 consumers now follow the same pattern (createPortal(..., document.body)) — consistent and idiomatic. The if (!open) guards remain where needed to prevent unnecessary portal creation, but the !portalTarget guard is correctly gone. ✅
Reviewed at 67a60c92.
applyThemePreference() was ported from the platform repo using
classList.toggle('dark'), but tokens.css expects [data-theme='dark']
attribute selectors. This meant dark/velvet mode tokens and all 304
dark: Tailwind utilities never activated.
- Set data-theme attribute on <html> instead of toggling CSS classes
- Update document viewer isDark checks to use dataset.theme
- Update OAuth page CSS from :root.dark to :root[data-theme='dark']
Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
devin-ai-integration
Bot
dismissed
vex-assistant-bot[bot]’s stale review
via
1fdbfca
devin-ai-integration
Bot
changed the title
ashleeradka
deleted the
devin/1779302850-lum-1711-remove-app-root-provider
branch
Test Results — LUM-1711Session: https://app.devin.ai/sessions/0a7470e554b2479db3dd1f2bb8f5a53d SummaryRan Vite dev server locally. Tested theme selector fix by toggling ResultsTheme selector fix (3/3 passed)
Portal fix (untested — requires auth)6 consumers (TrustRulesModal, TrustRuleFormModal, AttachmentPreviewModal, AvatarManagementModal, CommandPalette, ContextWindowIndicator) changed from Console Evidence |
…rollbars (#31387) The initial port of platform appTheme.css + globals.css to `packages/design-library/src/tokens.css` only bridged `--color-background`, `--color-foreground`, `--font-sans`, and the radius scale — but ~500 utility uses across the web app and design library reference moss/forest/danger/amber/stone Tailwind color scales and a handful of layout/animation classes that weren't carried over. After #31324 fixed the `dark:` selector wiring, those utilities activated but resolved to undefined variables, so half the palette silently no-op'd. Tier 1 — restore missing tokens in `tokens.css`: - Color scales (moss/stone/forest/emerald/danger/amber) with Figma hex values inside `@theme inline` so Tailwind generates utilities - Font tokens (`--font-mono`, `--font-serif`, `--font-display`, `--font-body`) - Shadow scale (`--shadow-sm/md/lg/glow/accent-glow`) - `--app-spacing-xxs..xxxl` - `--chat-max-width: 800px` - Missing velvet tokens (`--system-info-*`, `--feed-*`) - Explicit dark/velvet `--ring` so it tracks per-mode positive accent even if `data-theme` ever moves off the root Tier 2 — restore missing app-level classes in `apps/web/src/index.css`: - `.busy-indicator` + `busy-pulse` keyframe - `.onboarding-avatar-pulse/awake/failed` + `morphPulse` keyframe - `.app-shell` / `.app-shell-main` layout guards - Themed scrollbars rekeyed to `data-theme` (was `.dark .app-root`) - `prefers-reduced-motion` overrides for busy + typing indicators Misc: - Fix `--app-spacing-2xl` → `--app-spacing-xxl` typo in home-page.tsx - Fix stale "defined in appTheme.css" comment in busy-indicator.tsx - Drop "Keep in sync with platform appTheme.css" header — direction reversed post-cutover Architectural follow-up (NOT in this PR): a chunk of the restored color-scale utilities could collapse to semantic surface tokens (`bg-[var(--surface-lift)]` instead of `bg-stone-100 dark:bg-moss-700`). That's a ~500-site refactor and a separate decision; tracking separately. Co-authored-by: Claude <noreply@anthropic.com>
…1389) PR #31324 deleted AppRootProvider after confirming zero CSS rules target `.app-root` in this repo — theme tokens live on `:root` and apply via `data-theme` on `<html>`. Two `className="app-root"` strings survived that cleanup and are dead (target no CSS rule). The portal-container docstring example also still referenced the old wrapper pattern; it now reflects current conventions where overlays fall back to `document.body` directly and the provider is reserved for nested overlay scoping (dialogs, shadow DOM). Closes LUM-1769 Co-authored-by: Claude <noreply@anthropic.com>
1 participant
Prompt / plan
Inventory and fix vestigial platform-era patterns in the web app. LUM-1711 identified
AppRootProvideras never mounted; investigation revealed a second bug — theme selectors were misaligned between runtime and design library.Closes LUM-1711
What
Deletes the vestigial
AppRootProvider/useAppRootContainer()pattern and fixes the theme selector mismatch betweenapplyThemePreference()andtokens.css.Why
Two bugs, both caused by blindly porting platform repo patterns into the new Vite SPA without verifying they still apply:
Bug 1: 6 portal-based components silently rendering nothing
AppRootProviderprovides a scoped DOM element forcreatePortal(). It was ported from the platform repo but never mounted in the route tree. All 6 consumers calleduseAppRootContainer()→ gotnull→ hitif (!portalTarget) return nullguards → silently rendered nothing:TrustRulesModalTrustRuleFormModalAttachmentPreviewModalAvatarManagementModalCommandPalette(desktop overlay)ContextWindowIndicator(tooltip)The provider existed so portals would land inside a
.app-rootdiv where theme CSS variables resolve. In the new repo, all theme tokens are on:root/[data-theme]— there is no.app-rootCSS scoping. Portals todocument.bodyinherit all tokens via normal CSS inheritance. The provider is unnecessary.Bug 2: Dark/velvet mode completely broken
applyThemePreference()was copied verbatim from the platform repo usingclassList.toggle("dark"). Buttokens.cssin@vellum/design-libraryexpects[data-theme="dark"]attribute selectors, and the Tailwind v4 dark variant is wired to the same attribute:No code anywhere set the
data-themeattribute → all dark/velvet CSS custom properties never activated → all 304dark:Tailwind utility classes were non-functional.Changes
Portal fix (commit 1):
apps/web/src/components/app-root-context.tsxuseAppRootContainer(), portal todocument.body, remove dead!portalTargetnull guardsTheme fix (commit 2):
applyThemePreference(): setdata-themeattribute on<html>instead of toggling CSS classes — aligns withtokens.css[data-theme="dark"]/[data-theme="velvet"]selectorsdataset.themeinstead ofclassList.contains("dark"):root.darkCSS selector to:root[data-theme="dark"]Safety
:rootand[data-theme]selectors intokens.cssapply to<html>, sodocument.bodyand all descendants inherit themfont-sansis set on<body>viaroot-layout.tsx— portaled content inherits it.app-rootCSS rules exist anywhere in the codebasedata-themeis the canonical selector strategy pertokens.css,@custom-variant dark, and the design library's Storybook configRoot cause analysis
appTheme.csswith class-based dark mode (.dark); the new repo usestokens.csswith attribute-based dark mode ([data-theme]). The provider was ported but never wired into the route tree.appTheme.cssclass-based selectors and the design library'stokens.cssattribute-based selectors are two different systems that were never reconciled.tokens.cssdocumentsdata-themeas the theming mechanism.applyThemePreference()doesn't set it. The mismatch is visible by reading both files.CONVENTIONS.md,STYLE_GUIDE.md,tokens.css, andAGENTS.mdare the source of truth, not the platform repo.Test plan
References
Link to Devin session: https://app.devin.ai/sessions/0a7470e554b2479db3dd1f2bb8f5a53d
Requested by: @ashleeradka