Skip to content

Fix demo_form_dialog to omit trigger when not provided - #6940

Open
masenf wants to merge 2 commits into
mainfrom
claude/fragment-render-demo-form-2j4sdh
Open

Fix demo_form_dialog to omit trigger when not provided#6940
masenf wants to merge 2 commits into
mainfrom
claude/fragment-render-demo-form-2j4sdh

Conversation

@masenf

@masenf masenf commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Type of change

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

Description

Fixed demo_form_dialog() to properly handle the case when no trigger is provided. Previously, the function would create an empty rx.fragment() as a fallback trigger, which caused React runtime warnings because Fragment components cannot receive the props and ref that base-ui's render prop forwards to them.

Changes:

  • Removed the automatic rx.fragment() fallback when trigger is None
  • Modified the dialog to conditionally render dialog.trigger() only when a trigger is explicitly provided
  • This prevents Fragment from being used as a render prop target, eliminating React warnings

Testing

  • Added unit tests in tests/units/reflex_components_internal/blocks/test_demo_form.py:
    • test_demo_form_dialog_omits_trigger_when_not_given(): Verifies that no trigger is rendered when none is provided
    • test_demo_form_dialog_renders_given_trigger(): Verifies that a provided trigger is properly rendered

Both tests pass and validate the fix works as intended.

https://claude.ai/code/session_01FcXXMZnAkNtwBj7v2bshXH

Review in cubic

`demo_form_dialog` fell back to `rx.fragment()` when no trigger was
passed, so the base-ui `Dialog.Trigger` compiled to
`render:(jsx(Fragment, ({})))`. A Fragment cannot accept the props and
ref that base-ui forwards through the render prop, which makes React
complain in the console.

Match the pattern already used by the dialog, drawer, preview card, and
collapsible namespaces: render the trigger only when one is provided.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FcXXMZnAkNtwBj7v2bshXH
@masenf
masenf requested a review from a team as a code owner August 24, 2026 23:21

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All reported issues were addressed across 2 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread tests/units/reflex_components_internal/blocks/test_demo_form.py Outdated
@codspeed-hq

codspeed-hq Bot commented Aug 24, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 27 untouched benchmarks
⏩ 8 skipped benchmarks1


Comparing claude/fragment-render-demo-form-2j4sdh (7604652) with main (6b44604)

Open in CodSpeed

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.

@greptile-apps

greptile-apps Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR updates demo_form_dialog to omit dialog.trigger when no trigger is supplied, avoiding an invalid Fragment render-prop target.

  • Conditionally includes the dialog trigger only for an explicitly supplied component.
  • Adds tests covering dialogs with and without a trigger.

Confidence Score: 4/5

The PR should not merge until the outstanding import-order violation in the new test file is corrected so the lint gate passes.

The functional fix is focused, but the new test file still has the import ordering reported in the prior thread, which violates the repository’s isort convention and is rejected or modified by the configured Ruff check.

Files Needing Attention: tests/units/reflex_components_internal/blocks/test_demo_form.py

Important Files Changed

Filename Overview
packages/reflex-components-internal/src/reflex_components_internal/blocks/demo_form.py Conditionally renders dialog.trigger so triggerless dialogs no longer create an empty Fragment render target.
tests/units/reflex_components_internal/blocks/test_demo_form.py Adds focused trigger-rendering regression tests, but the outstanding import ordering causes the configured lint gate to reject or rewrite the file.

Reviews (2): Last reviewed commit: "test(internal): assert the demo form tri..." | Re-trigger Greptile

Comment thread tests/units/reflex_components_internal/blocks/test_demo_form.py
`Book a Demo` is also the dialog's own heading, so the assertion passed
even if the trigger's content were dropped. Use a label the dialog body
never renders, and assert it is absent without a trigger so the test
keeps guarding against a collision.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FcXXMZnAkNtwBj7v2bshXH
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