✅ Snyk checks have passed. No issues have been found so far.

Status	Scanner	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@cursor review

@KaranPradhan266 thanks for submitting this PR! I am wondering if you could look into whether there are some e2e tests that could be added for this? Since it is a visual change, I think it would be a good idea.

@sfc-gh-lwilby Sure, will look into it!

Also linking @jrieke for doing a quick product review

Summary

This PR adds optional icon support to @st.dialog and st._main._dialog(), allowing users to display an emoji or Material icon next to the dialog title. The implementation follows existing Streamlit patterns and touches all layers of the stack: proto definitions, Python backend, TypeScript frontend, and corresponding tests.

Key Changes:

• Added icon field (On rerun, faded elements are becoming opaque one by one. Instead they become opaque all at once. #6) to the Dialog proto message
• Extended dialog_decorator.py, layouts.py, and lib/dialog.py to accept and propagate the icon parameter
• Updated frontend Dialog.tsx to render icons using the existing DynamicIcon component
• Added styled components for icon layout (StyledDialogIcon)
• Added Python unit tests, frontend unit tests, and E2E tests

Code Quality

The implementation is clean, well-organized, and follows existing Streamlit patterns:

✅ Good Patterns Followed:

• Uses existing validate_icon_or_emoji for validation, which handles emojis, Material icons, and the special "spinner" value
• Leverages existing DynamicIcon component for frontend rendering
• Proper use of theme spacing in StyledDialogTitle (gap: theme.spacing.sm)
• Good docstring for the icon parameter in dialog_decorator.py (lines 231-235)
• Icon is optional with sensible default (None → empty string in proto)

✅ Proto Change (Block.proto:128):

• Field icon = 6 is a new optional field - this is a backwards-compatible change per protobuf rules
• Good comment explaining the field's purpose

✅ Frontend Code (Dialog.tsx:162-166):

• Conditional rendering pattern ({icon && ...}) is idiomatic React
• Test ID stDialogIcon follows Streamlit's naming convention (stComponentSubcomponent)
• Uses size="lg" for the icon which is appropriate for header context

✅ Styled Components (styled-components.ts:19-32):

• StyledDialogTitle properly uses flexbox for layout with theme-based spacing
• StyledDialogIcon includes flexShrink: 0 to prevent icon compression

Test Coverage

Python Unit Tests (layouts_test.py:1037-1060):

• ✅ test_dialog_sets_icon: Tests direct _dialog() API with icon
• ✅ test_dialog_decorator_sets_icon: Tests decorator API with icon
• Both tests verify the icon value propagates to the proto message correctly
• Tests include brief docstrings as per guidelines

Frontend Unit Tests (Dialog.test.tsx:84-106):

• ✅ renders an icon when provided: Verifies icon visibility and content
• ✅ does not render an icon when not provided: Verifies no icon element when prop is absent
• Uses screen.getByTestId / screen.queryByTestId appropriately
• Uses toBeVisible() instead of redundant toBeInTheDocument() as per best practices

E2E Tests (st_dialog_test.py:291-297):

• ✅ test_dialog_icon_is_displayed: Verifies icon is visible with correct text content
• Uses expect() for auto-wait assertions (not assert)
• Uses stable locators (get_by_role("dialog"), get_by_test_id("stDialogIcon"))

Minor Suggestions for Test Coverage:

1. Consider adding a test case using a Material icon shortcode (e.g., ":material/info:") in addition to the emoji test to verify the full icon system works with dialogs.
2. A snapshot test for the dialog with icon could help catch visual regressions in the icon + title layout.

Backwards Compatibility

✅ Fully Backwards Compatible:

• Proto field icon = 6 is a new optional string field with implicit default ""
• Python icon parameter defaults to None, maintaining existing API
• Frontend handles missing/empty icon gracefully with conditional rendering
• No breaking changes to existing dialog functionality

Security & Risk

✅ No Security Concerns:

• Icon validation uses existing validate_icon_or_emoji which restricts values to:
  • Valid single-character emojis from the allowed list
  • Valid Material Symbol shortcodes
  • The special "spinner" value
• No user-provided HTML/JS can be injected through the icon parameter

✅ Low Regression Risk:

• Changes are additive and don't modify existing behavior
• Existing tests for dialog functionality are preserved
• New icon styling uses flexbox which won't affect dialogs without icons

Recommendations

1. Consider Material icon test case (optional enhancement): Add an E2E or unit test using :material/info: syntax to ensure complete coverage of the icon system.

2. Consider snapshot test (optional enhancement): A snapshot test for st_dialog-with_icon would catch visual regressions in the icon + title layout.

3. Example in docstring (nice to have): The main example in the docstring could show icon usage alongside the existing vote example, though this is minor since the parameter documentation is clear.

Verdict

APPROVED: This is a well-implemented feature that follows Streamlit patterns, includes appropriate test coverage across all layers (Python, TypeScript, E2E), maintains backwards compatibility, and poses no security risks. The suggestions above are optional enhancements rather than required changes.

This is an automated AI review. Please verify the feedback and use your judgment.

@lukasmasuch @sfc-gh-lwilby In my next commit, I’ll be covering updates to the icon comment descriptions, adding snapshot tests to verify both material-style and emoji icons, and adding a test to assert that the spinner icon’s test ID is visible. In addition to these updates, let me know if there is anything else you’d like me to address?

@sfc-gh-lmasuch @sfc-gh-lwilby @jrieke Let me know if these look good!

LGTM from product/design side!

@KaranPradhan266 it looks like you will need to commit your snapshots that you added now. You can run make update-snapshots locally and then commit the results. I will do a final review later today or tomorrow.

@sfc-gh-lwilby @lukasmasuch I looked into the failing tests and none of the failures appear to be caused by changes in this PR.

The Python unit test failure seems related to LoginHandlerTest, unlikely because of curent changes. For Playwright e2e, the failure is isolated to test_dialog_with_chart and occurs only on Firefox. This looks like test flakiness due to using a fixed hover position ({x: 80, y: 200}), not the dialog icon changes.

We can make the test more deterministic by hovering using the chart’s bounding box instead. I can push a fix for the test if you’d like, but it would be unrelated to this PR. Let me know how you’d like to proceed.

Thanks for looking into these failures @KaranPradhan266 . Apologies for the flaky test, unfortunately, it seems like maybe adding more elements into the test file for your tests is triggering the failure in this other test even though that test already had this flaw. If you have time to fix this other test it will help merge your PR faster. We can't merge it with the test failing because it will break develop.

For the python unit test, I agree it doesn't apper related to your changes exactly, but it seems to also be persistently failing so it needs to be resolved before merging into the develop to keep develop green. I wonder if something went wrong when you merged in develop and fixed the merge conflict.

I can try to take a look if you don't have time, but if you have time to resolve these fixes that will likely be the fastest solution because I have some other priorities to attend to first.

Thanks again! Sorry these test failures are getting in the way of merging your PR.

@KaranPradhan266 an update on the LoginHandlerTest failure. This is an issue affecting other PRs that was introduced during a dependency update and we are working on a fix. So it is just the e2e test failure.

@sfc-gh-lwilby Thanks for the update! I’ve pushed the latest changes, which should now handle the e2e failures as well. Please let me know if you still see any issues on your end.

@lukasmasuch My apologies for the oversight on type checker part, resolved it!

@lukasmasuch @sfc-gh-lwilby I believe all feedback has been addressed, can you confirm whether this PR is ready to merge, or if there’s anything else needed?