Skip to content

Add parent-child validation for Radix Dialog components#5894

Closed
devin-ai-integration[bot] wants to merge 1 commit intomainfrom
devin/1760638517-add-radix-dialog-validation
Closed

Add parent-child validation for Radix Dialog components#5894
devin-ai-integration[bot] wants to merge 1 commit intomainfrom
devin/1760638517-add-radix-dialog-validation

Conversation

@devin-ai-integration
Copy link
Copy Markdown
Contributor

Summary

Adds parent-child validation for Radix UI components to catch incorrect component nesting at compile time instead of runtime. Fixes the error: DialogTrigger must be used within Dialog.

Changes

Added _valid_parents ClassVar to the following Radix components:

Dialog (primitives & themes):

  • DialogTrigger → must be child of DialogRoot
  • DialogPortal → must be child of DialogRoot
  • DialogOverlay → must be child of DialogRoot or DialogPortal
  • DialogContent → must be child of DialogRoot or DialogPortal
  • DialogTitle, DialogDescription, DialogClose → must be child of DialogRoot, DialogPortal, or DialogContent

AlertDialog (themes):

  • AlertDialogTrigger → must be child of AlertDialogRoot
  • AlertDialogContent → must be child of AlertDialogRoot
  • AlertDialogTitle, AlertDialogDescription, AlertDialogAction, AlertDialogCancel → must be child of AlertDialogRoot or AlertDialogContent

Drawer (primitives):

  • DrawerTrigger, DrawerPortal → must be child of DrawerRoot
  • DrawerOverlay, DrawerContent → must be child of DrawerRoot or DrawerPortal
  • DrawerTitle, DrawerDescription, DrawerClose, DrawerHandle → must be child of DrawerRoot, DrawerPortal, or DrawerContent

Popover & HoverCard (themes):

  • PopoverTrigger, PopoverContent → must be child of PopoverRoot
  • PopoverClose → must be child of PopoverRoot or PopoverContent
  • HoverCardTrigger, HoverCardContent → must be child of HoverCardRoot

Testing

Added comprehensive test suite (test_dialog_validation.py) with 15 test cases covering:

  • Invalid parent scenarios (should raise ValueError)
  • Valid parent scenarios (should succeed)
  • Full component structure validation

All tests pass ✅

Notes

  • Pre-existing pyright error: There's a pre-existing import error for RadixPrimitiveTriggerComponent in dialog.py that exists on the main branch (verified). This PR does not introduce new pyright errors.
  • Breaking change: This will cause ValueError at component creation time for users who were incorrectly nesting these components. However, this is actually fixing broken code - these components would fail at runtime in React anyway.

Checklist

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

  • Bug fix (non-breaking change which fixes an issue)

New Feature Submission:

  • Does your submission pass the tests?
  • Have you linted your code locally prior to submission?

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Human Review Checklist

Reviewers should verify:

  1. Parent-child relationships are correct - Compare with Radix UI documentation to ensure validation rules match Radix's requirements
  2. Test coverage is adequate - All critical parent-child relationships are tested
  3. Other Radix components - Are there other Radix components (ContextMenu, DropdownMenu, etc.) that need similar validation?
  4. Breaking change impact - Consider documenting this in release notes as it will catch previously-silent errors

Link to Devin run: https://app.devin.ai/sessions/23c5a495b9274b09a0cc661bca2928ce
Requested by: Alek (@Alek99, alek@reflex.dev)

…pover, and HoverCard components

- Add _valid_parents ClassVar to Dialog components (primitives and themes)
- Add _valid_parents ClassVar to AlertDialog components
- Add _valid_parents ClassVar to Drawer components
- Add _valid_parents ClassVar to Popover and HoverCard components
- Add comprehensive test suite for Dialog validation
- Fix the error: DialogTrigger must be used within Dialog

This prevents runtime React errors by validating component hierarchy at compile time.

Note: Pre-existing pyright error in dialog.py (RadixPrimitiveTriggerComponent import)
exists on main branch and is not introduced by these changes.

Co-Authored-By: Alek <alek@pynecone.io>
@devin-ai-integration
Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Overview

Summary

Adds compile-time parent-child validation for Radix UI components using the _valid_parents ClassVar pattern to catch incorrect component nesting early.

Key Changes:

  • Added _valid_parents validation to Dialog (primitives & themes), AlertDialog, Drawer, Popover, and HoverCard components
  • Primitives allow more flexible parent relationships (including DialogRoot, DialogPortal, DialogContent)
  • Themes enforce stricter hierarchies (typically just Root and Content as valid parents)
  • Validation leverages existing _iter_parent_classes_names() mechanism in base Component class
  • Test suite added for Dialog components with 15 test cases covering both invalid and valid scenarios

Architecture:
The validation works by checking if any class in the parent's MRO (method resolution order) matches the child's _valid_parents list. This catches errors at component creation time rather than React runtime, providing better developer experience.

Test Coverage Gap:
Tests only cover Dialog components. AlertDialog, Drawer, Popover, and HoverCard validation rules are untested, though they follow the same pattern.

Confidence Score: 4/5

  • Safe to merge with minor test coverage gaps for non-Dialog components
  • The implementation correctly uses the existing validation framework and follows established patterns in the codebase (e.g., AccordionRoot pattern). Parent-child relationships match Radix UI's expected hierarchies. However, losing one point because tests only cover Dialog components while the PR modifies 5 different component families (Dialog, AlertDialog, Drawer, Popover, HoverCard).
  • tests/units/components/radix/test_dialog_validation.py - Consider adding test coverage for AlertDialog, Drawer, Popover, and HoverCard validation rules

Important Files Changed

File Analysis

Filename Score Overview
reflex/components/radix/primitives/dialog.py 5/5 Added _valid_parents ClassVar to Dialog primitive components (DialogPortal, DialogOverlay, DialogTrigger, DialogContent, DialogTitle, DialogDescription, DialogClose) to enforce parent-child validation
reflex/components/radix/primitives/drawer.py 5/5 Added _valid_parents ClassVar to Drawer primitive components (DrawerTrigger, DrawerPortal, DrawerContent, DrawerOverlay, DrawerClose, DrawerTitle, DrawerDescription, DrawerHandle)
reflex/components/radix/themes/components/dialog.py 5/5 Added _valid_parents ClassVar to themed Dialog components (DialogTrigger, DialogTitle, DialogContent, DialogDescription, DialogClose) with more restrictive rules than primitives
reflex/components/radix/themes/components/alert_dialog.py 5/5 Added _valid_parents ClassVar to AlertDialog components (AlertDialogTrigger, AlertDialogContent, AlertDialogTitle, AlertDialogDescription, AlertDialogAction, AlertDialogCancel)
tests/units/components/radix/test_dialog_validation.py 4/5 Added comprehensive test suite with 15 test cases for both primitive and themed Dialog components. Tests cover invalid and valid parent scenarios. Missing tests for AlertDialog, Drawer, Popover, and HoverCard.

Sequence Diagram

sequenceDiagram
    participant User
    participant Component
    participant DialogRoot
    participant ValidationLogic
    participant DialogTrigger
    
    User->>Component: Create DialogTrigger.create()
    Component->>DialogTrigger: Initialize component
    DialogTrigger->>DialogTrigger: Check _valid_parents ClassVar
    Note over DialogTrigger: _valid_parents = ["DialogRoot"]
    
    alt Parent is DialogRoot
        Component->>DialogRoot: Add DialogTrigger as child
        DialogRoot->>ValidationLogic: Validate child component
        ValidationLogic->>ValidationLogic: Call _iter_parent_classes_names()
        ValidationLogic->>ValidationLogic: Check if "DialogRoot" in _valid_parents
        ValidationLogic-->>DialogRoot: Validation passed ✓
        DialogRoot-->>User: Component created successfully
    else Parent is Box (Invalid)
        Component->>ValidationLogic: Validate child component
        ValidationLogic->>ValidationLogic: Check if parent in _valid_parents
        ValidationLogic->>ValidationLogic: "Box" not in ["DialogRoot"]
        ValidationLogic-->>Component: Raise ValueError
        Component-->>User: Error: DialogTrigger must be child of DialogRoot
    end
Loading

8 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Oct 16, 2025

CodSpeed Performance Report

Merging #5894 will create unknown performance changes

Comparing devin/1760638517-add-radix-dialog-validation (517681e) with main (fc0a6ca)

Summary

⚠️ No benchmarks were detected in both the base of the PR and the PR.
Please ensure that your benchmarks are correctly instrumented with CodSpeed.

Check out the benchmarks creation guide

⏩ 8 skipped1

Footnotes

  1. 8 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@adhami3310
Copy link
Copy Markdown
Member

this is overly restrictive, evidenced by reflex-web failing, it happens that the restriction of "valid parents" is that it should go up the container, because radix just demands the context of it to be there

@adhami3310 adhami3310 closed this Oct 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants