Skip to content

[refactor] Log widget state duplication warning to console instead of UI#14141

Merged
lukasmasuch merged 1 commit intodevelopfrom
lukasmasuch/log-widget-dup-warn
Mar 2, 2026
Merged

[refactor] Log widget state duplication warning to console instead of UI#14141
lukasmasuch merged 1 commit intodevelopfrom
lukasmasuch/log-widget-dup-warn

Conversation

@lukasmasuch
Copy link
Copy Markdown
Collaborator

@lukasmasuch lukasmasuch commented Feb 26, 2026 •
edited
Loading

Describe your changes

Changes widget state duplication warning from st.warning() (UI display) to _LOGGER.warning() (console output), reducing visual noise while still informing developers.

  • Warning now logs to console instead of showing in app UI
  • Added stack_info=True for better debugging (shows where warning originates)

Testing Plan

  • Unit Tests (Python)

@lukasmasuch
Log widget state duplication warning to console
e5c8bcb 
Change widget state duplication warning from st.warning() to
_LOGGER.warning() to reduce UI clutter while still informing
developers in the console. Added stack_info for easier debugging.
@lukasmasuch lukasmasuch added change:refactor PR contains code refactoring without behavior change impact:users PR changes affect end users ai-review If applied to PR or issue will run AI review workflow labels Feb 26, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 26, 2026 •
edited
Loading

✅ PR preview is ready!

Name Link
📦 Wheel file https://core-previews.s3-us-west-2.amazonaws.com/pr-14141/streamlit-1.54.0-py3-none-any.whl
📦 @streamlit/component-v2-lib Download from artifacts
🕹️ Preview app pr-14141.streamlit.app (☁️ Deploy here if not accessible)

@snyk-io
Copy link
Copy Markdown
Contributor

snyk-io bot commented Feb 26, 2026 •
edited
Loading

✅ 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.

@github-actions github-actions bot removed the ai-review If applied to PR or issue will run AI review workflow label Feb 26, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 26, 2026

Consolidated Code Review

Summary

This PR replaces the widget state duplication warning from a user-facing st.warning() call (rendered in the app UI) to a backend _LOGGER.warning() call (logged to the console/server logs). The config description in config.py is updated accordingly ("logs" instead of "displays"), the docstring in policies.py receives an accuracy improvement, and all related unit tests are updated to assert logger behavior. stack_info=True is added for improved debugging context.

Reviewer Consensus

Both reviewers (gpt-5.3-codex-high and opus-4.6-thinking) approved this PR and are in strong agreement on all key points:

Aspect Agreement
Change is clean and low-risk Full agreement
Logging pattern follows existing codebase conventions (maybe_raise_label_warnings) Full agreement
Test coverage is adequate Full agreement
No backwards compatibility issues Full agreement
No security concerns Full agreement
No accessibility regressions Full agreement

Code Quality

  • Consistent logging pattern: The _LOGGER.warning(..., stack_info=True) call matches the identical pattern already used in maybe_raise_label_warnings() in the same file (lines 187-193), confirmed by code inspection. Both reviewers noted this.
  • Proper lazy formatting: The change from f-string to %s-style logging arguments is the correct approach for Python logging, avoiding unnecessary string interpolation when the log level is suppressed.
  • Docstring improvement: The Raises section was updated from the generic StreamlitAPIException to the specific StreamlitValueAssignmentNotAllowedError, which accurately matches the only exception raised.
  • Test bug fix (noted only by opus-4.6-thinking): The old test reset utils._shown_default_value_warning = False, but the actual flag lives in policies.py. The new test correctly imports policies_module and resets policies_module._shown_default_value_warning = False. This is a genuine bug fix in the test — the old code was inadvertently creating a new attribute on the utils module rather than resetting the intended flag.
  • Typo fix (noted only by opus-4.6-thinking): A test comment was corrected from "Reset globale flag:" to "Reset global flag:".

Test Coverage

Tests are well-updated to reflect the new behavior:

  • All existing test cases (no-key, no-value, no-state-value, disabled-warning, writes-not-allowed) are preserved and correctly updated to mock _LOGGER instead of st.warning.
  • The positive test verifies: (1) the warning is called once, (2) the format string and key argument are correct, and (3) stack_info=True is passed.
  • The unused from streamlit.elements.lib import utils import is correctly removed.

Minor gap (noted by gpt-5.3-codex-high): No E2E test was added to assert the warning is no longer rendered in the app UI. This is a reasonable follow-up suggestion but not blocking, as the code change from st.warning to _LOGGER.warning makes the behavior clear.

Backwards Compatibility

No breaking API changes. The global.disableWidgetStateDuplicationWarning config option continues to work as before. The only behavioral change is intentional: duplication warnings go to logs instead of the app UI. Users who relied on seeing this warning in the UI will need to check console logs instead.

Minor Suggestions (Non-blocking)

  1. Function-local import (raised by gpt-5.3-codex-high): The test at line 162 uses import streamlit.elements.lib.policies as policies_module inside the test function. However, after verification, this is a reasonable pattern here — the import is needed specifically to reset a module-level flag (_shown_default_value_warning), and keeping it local makes the intent explicit. Other test files in the codebase use similar patterns. Verdict: acceptable as-is, but moving to module scope would also be fine.

  2. Optional E2E follow-up (raised by gpt-5.3-codex-high): An E2E test to confirm the warning no longer appears in the rendered app could be a useful follow-up but is not required for this PR.

Security & Risk

No security concerns. Risk is minimal — the logic flow is identical, only the output destination changes.

Verdict

APPROVED: Both reviewers approved. The refactoring is clean, well-tested, follows existing codebase patterns, and includes a subtle bug fix in the test code. No blocking issues were identified.

Consolidated review by opus-4.6-thinking. Individual reviews below.

📋 Review by `gpt-5.3-codex-high`

Summary

This PR replaces the widget state duplication warning from a UI-visible st.warning() call to a backend logger warning in check_session_state_rules, and updates the related config description text. The unit tests were adjusted accordingly to assert logger behavior (including stack_info=True) instead of UI warning emission.

Code Quality

No functional defects were identified in the changed logic. The refactor aligns with lib/streamlit/AGENTS.md logging guidance by using module-level _LOGGER.warning(...) in lib/streamlit/elements/lib/policies.py.

Minor style issue (non-blocking):

  • lib/tests/streamlit/elements/element_policies_test.py:162 introduces a function-local import (import streamlit.elements.lib.policies as policies_module) where a top-level import would better follow lib/tests/AGENTS.md guidance.

Test Coverage

Unit coverage for the modified behavior is solid:

  • Tests verify warning is not logged for no key/no default/no new session-state value and when the duplication warning is disabled.
  • Tests verify the warning is logged once with lazy formatting args and stack_info=True.
  • Existing write-protection behavior (StreamlitValueAssignmentNotAllowedError) remains covered.

No e2e test was added for the UI behavior change (warning no longer rendered in-app). This is a small residual gap, but the direct call-site change from st.warning to _LOGGER.warning keeps regression risk low.

Backwards Compatibility

No API or config-key breaking change was introduced. The main behavioral change is intentional: duplication warnings now go to logs instead of the app UI. Existing apps continue to run, and global.disableWidgetStateDuplicationWarning still controls warning emission.

Security & Risk

No security concerns found. Risk is low and mostly limited to observability expectations (developers relying on UI warning visibility will now need to inspect logs).

Accessibility

No new frontend UI was introduced. Removing an in-app warning from this path reduces visual noise and does not introduce accessibility regressions.

Recommendations

  1. Move the function-local import in lib/tests/streamlit/elements/element_policies_test.py to module scope to align with test-file import conventions.
  2. Optionally add an e2e regression test in a follow-up PR to assert the warning is not surfaced in the rendered app UI.

Verdict

APPROVED: The refactor is clean, low-risk, and adequately covered by updated unit tests, with only minor non-blocking follow-up suggestions.

This is an automated AI review by gpt-5.3-codex-high. Please verify the feedback and use your judgment.

📋 Review by `opus-4.6-thinking`

Summary

This PR changes the widget state duplication warning from st.warning() (which renders in the app UI) to _LOGGER.warning() (which logs to the console). This reduces visual noise for end users while still informing developers about the issue. The config description in config.py is updated to match ("logs" instead of "displays"), and the docstring in policies.py is improved with a more accurate exception description. stack_info=True is added for better debugging context.

Code Quality

The code is clean and follows existing patterns well:

  • Consistent logging pattern: The new _LOGGER.warning() call with stack_info=True matches the exact pattern already used in maybe_raise_label_warnings() in the same file (policies.py, lines 187-193), making this a natural fit.
  • Proper lazy formatting: The change from f-string (f'The widget with key "{key}"...') to %s-style ('The widget with key "%s"...', key`) is the correct approach for Python logging, avoiding string interpolation when the log message may be suppressed.
  • Docstring improvement: The Raises section was updated from the generic StreamlitAPIException to the specific StreamlitValueAssignmentNotAllowedError, which accurately describes the only exception the function raises.
  • Test bug fix: The old test incorrectly reset utils._shown_default_value_warning = False (writing to the utils module), but the actual flag lives in policies.py. The new test correctly imports policies_module and resets policies_module._shown_default_value_warning = False. This is a subtle but real bug fix in the test — the old code was creating a new attribute on utils rather than resetting the flag in policies.

No issues found with code structure or maintainability.

Test Coverage

Tests are well-updated to reflect the new behavior:

  • All existing test cases (no-key, no-value, no-state-value, disabled-warning, writes-not-allowed) are preserved and correctly updated to mock _LOGGER instead of st.warning.
  • The positive test (test_check_session_state_rules_prints_warning) verifies three things: the warning is called once, the format string and key argument are correct, and stack_info=True is passed.
  • The unused from streamlit.elements.lib import utils import is correctly removed.
  • No E2E tests are affected since the warning was previously a UI element and is now console-only — there are no existing E2E tests for this specific warning behavior.

Coverage is adequate for this change.

Backwards Compatibility

This is a minor behavioral change for users:

  • Before: The widget state duplication warning appeared as an st.warning() element in the app UI.
  • After: The warning is logged to the console/server logs only.

This is intentionally a UX improvement (reducing visual noise). Users who relied on seeing this warning in the UI will need to check console logs instead. The global.disableWidgetStateDuplicationWarning config option continues to work as before. The config description is updated to say "logs" instead of "displays," which is accurate.

No breaking API changes. No protobuf changes. No frontend changes needed.

Security & Risk

No security concerns. The change is a straightforward swap from UI display to console logging. The warning message content is unchanged. The stack_info=True parameter only adds stack trace information to the log output, which is helpful for debugging and poses no security risk.

Risk of regression is minimal — the logic flow is identical, only the output destination changes.

Accessibility

No frontend changes are involved, so accessibility is not affected. Removing a UI warning element could arguably be a minor accessibility improvement since it reduces unexpected content changes in the app.

Recommendations

No blocking issues. One optional suggestion:

  1. Optional — verify typo fix is intentional: The test comment was changed from "Reset globale flag:" to "Reset global flag:" — this is a good typo fix, just noting it for completeness.

Verdict

APPROVED: Clean, well-tested refactoring that moves the widget state duplication warning from UI display to console logging, following existing patterns in the codebase and fixing a minor pre-existing test bug.

This is an automated AI review by opus-4.6-thinking.

@lukasmasuch lukasmasuch changed the title [refactor] Log widget state duplication warning to console [refactor] Log widget state duplication warning to console instead of UI Feb 26, 2026
@lukasmasuch lukasmasuch marked this pull request as ready for review February 26, 2026 16:35
Copilot AI review requested due to automatic review settings February 26, 2026 16:35
Copilot AI reviewed Feb 26, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the widget state duplication warning from a UI-displayed warning (st.warning()) to a console-logged warning (_LOGGER.warning()), reducing visual clutter in apps while still alerting developers to potential configuration issues. The change adds stack_info=True for better debugging capabilities, helping developers trace where the warning originates.

Changes:

  • Changed warning output method from UI display to console logging for widget state duplication
  • Added stack trace information to the warning for improved debugging
  • Updated all related tests to mock and verify the logger instead of the UI warning function

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
lib/streamlit/elements/lib/policies.py Changed st.warning() to _LOGGER.warning() with stack_info=True and updated the warning message format to use string formatting with positional arguments
lib/tests/streamlit/elements/element_policies_test.py Updated all test mocks from streamlit.warning to streamlit.elements.lib.policies._LOGGER, removed unused import, fixed typo ("globale" -> "global"), and added assertions to verify logger call arguments including stack_info
lib/streamlit/config.py Updated documentation to reflect that the warning is now logged to console rather than displayed in the UI

@lukasmasuch lukasmasuch merged commit 600127c into develop Mar 2, 2026
92 of 96 checks passed
@lukasmasuch lukasmasuch deleted the lukasmasuch/log-widget-dup-warn branch March 2, 2026 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@sfc-gh-bnisco sfc-gh-bnisco sfc-gh-bnisco approved these changes

Assignees

No one assigned

Labels

change:refactor PR contains code refactoring without behavior change impact:users PR changes affect end users

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lukasmasuch @sfc-gh-bnisco