ENG-10963 refactor(log): migrate reflex-base and component packages to logging (2/5) - #6864
Conversation
Replace console.debug/info/warn/error call sites with per-module logging.getLogger(__name__) loggers across reflex-base and the component packages. No behavior change beyond the new sink.
Greptile SummaryThe PR migrates user-facing diagnostics in reflex-base and several component packages from legacy console helpers to module-scoped standard-library loggers without changing their functional branches.
Confidence Score: 5/5The PR appears safe to merge, with the migrated logging paths retaining handler initialization, level handling, deduplication, and exception traceback behavior. The changed calls preserve their existing control flow and observable diagnostics through the configured logging pipeline, and no blocking or independently actionable non-blocking issue remains.
|
| Filename | Overview |
|---|---|
| packages/reflex-base/src/reflex_base/config.py | Migrates configuration diagnostics and ensures the logging pipeline is configured for the default-level branch; no actionable defect was identified. |
| packages/reflex-base/src/reflex_base/event/processor/event_processor.py | Replaces manually formatted tracebacks with logger.exception inside active exception handlers, preserving exception details. |
| packages/reflex-base/src/reflex_base/plugins/sitemap.py | Migrates sitemap warnings to standard logging while preserving their conditions and message content. |
| packages/reflex-components-plotly/src/reflex_components_plotly/plotly.py | Migrates the optional-dependency warning to a bootstrapped package logger. |
| tests/units/test_config.py | Updates warning assertions to use pytest log capture with level-aware checks. |
| tests/units/plugins/test_sitemap.py | Updates sitemap warning tests to validate captured warning records and exact messages. |
Reviews (1): Last reviewed commit: "refactor(log): migrate reflex-base and c..." | Re-trigger Greptile
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 666df42e4f
βΉοΈ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with π.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| except ImportError: | ||
| console.warn("Plotly is not installed. Please run `pip install plotly`.") | ||
| logger.warning("Plotly is not installed. Please run `pip install plotly`.") |
There was a problem hiding this comment.
Bootstrap logging for direct component-package imports
When this independently published package is imported directly rather than through import reflex, the only _log.bootstrap() call in reflex/__init__.py never runs. If Plotly is absent, this migrated warning therefore falls through to logging's plain stderr fallback instead of the Reflex handlers, so options such as REFLEX_LOG_JSON and REFLEX_ENABLE_FULL_LOGGING are ignored and can corrupt an otherwise machine-readable output stream. Ensure the Reflex logging pipeline is bootstrapped for standalone component-package imports before emitting these records.
Useful? React with πΒ / π.
There was a problem hiding this comment.
4 issues found across 28 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid β if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/reflex-components-react-player/news/+eng-10963-logging.misc.md">
<violation number="1" location="packages/reflex-components-react-player/news/+eng-10963-logging.misc.md:1">
P3: This news fragment uses the orphan `+` prefix rather than the PR number. CONTRIBUTING.md says orphan fragments should be renamed after the PR opens; left as-is, towncrier materializes the changelog entry against a meaningless `+` reference instead of the issue/PR link, which weakens traceability in the release notes. Renaming to `<pr>.misc.md` clears this up.</violation>
</file>
<file name="packages/reflex-base/src/reflex_base/event/__init__.py">
<violation number="1" location="packages/reflex-base/src/reflex_base/event/__init__.py:1993">
P3: The local `from reflex_base.utils import console` import in this function is now unused after the console.warn β logger.warning change β the import is the only remaining reference. Remove it to avoid a dead import (and any linter F401 warning).</violation>
</file>
<file name="tests/units/plugins/test_sitemap.py">
<violation number="1" location="tests/units/plugins/test_sitemap.py:196">
P3: The rewritten dynamic-route and 404-route assertions count WARNING-level records across every logger via caplog (`len(warnings) == 1`), not just the sitemap plugin's logger. caplog captures records propagated to the root from all loggers, so an unrelated WARNING logged elsewhere while the test runs (a third-party warning, or a future warning added elsewhere in the sitemap flow) would make these exact-count assertions fail spuriously, and the assertion no longer isolates the behavior under test. Consider filtering by the plugin's logger name (e.g. `r.name == "reflex_base.plugins.sitemap"`) or asserting on `[r for r in caplog.records if "does not have a 'loc'" in r.getMessage()]` so the tests stay robust and focused on the sitemap module's own warning.</violation>
</file>
<file name="packages/reflex-components-plotly/src/reflex_components_plotly/plotly.py">
<violation number="1" location="packages/reflex-components-plotly/src/reflex_components_plotly/plotly.py:21">
P2: Since `reflex_components_plotly` can be installed and imported standalone (without `import reflex`), this warning is emitted via a module logger that may never get the Reflex logging handlers attached (those are only wired up through the Reflex config bootstrap). In that case the warning falls back to Python logging's default stderr handler, bypassing options like `REFLEX_LOG_JSON`/`REFLEX_ENABLE_FULL_LOGGING` and potentially polluting a JSON-structured log stream with a plain-text line. Consider ensuring the logging pipeline is configured (or lazily bootstrapped) before emitting logs from standalone component packages.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| except ImportError: | ||
| console.warn("Plotly is not installed. Please run `pip install plotly`.") | ||
| logger.warning("Plotly is not installed. Please run `pip install plotly`.") |
There was a problem hiding this comment.
P2: Since reflex_components_plotly can be installed and imported standalone (without import reflex), this warning is emitted via a module logger that may never get the Reflex logging handlers attached (those are only wired up through the Reflex config bootstrap). In that case the warning falls back to Python logging's default stderr handler, bypassing options like REFLEX_LOG_JSON/REFLEX_ENABLE_FULL_LOGGING and potentially polluting a JSON-structured log stream with a plain-text line. Consider ensuring the logging pipeline is configured (or lazily bootstrapped) before emitting logs from standalone component packages.
Prompt for AI agents
Check if this issue is valid β if so, understand the root cause and fix it. At packages/reflex-components-plotly/src/reflex_components_plotly/plotly.py, line 21:
<comment>Since `reflex_components_plotly` can be installed and imported standalone (without `import reflex`), this warning is emitted via a module logger that may never get the Reflex logging handlers attached (those are only wired up through the Reflex config bootstrap). In that case the warning falls back to Python logging's default stderr handler, bypassing options like `REFLEX_LOG_JSON`/`REFLEX_ENABLE_FULL_LOGGING` and potentially polluting a JSON-structured log stream with a plain-text line. Consider ensuring the logging pipeline is configured (or lazily bootstrapped) before emitting logs from standalone component packages.</comment>
<file context>
@@ -2,21 +2,23 @@
except ImportError:
- console.warn("Plotly is not installed. Please run `pip install plotly`.")
+ logger.warning("Plotly is not installed. Please run `pip install plotly`.")
if not TYPE_CHECKING:
Figure = Any
</file context>
| @@ -0,0 +1 @@ | |||
| Internal logging migrated from the legacy console helpers to standard python `logging` per-module loggers. | |||
There was a problem hiding this comment.
P3: This news fragment uses the orphan + prefix rather than the PR number. CONTRIBUTING.md says orphan fragments should be renamed after the PR opens; left as-is, towncrier materializes the changelog entry against a meaningless + reference instead of the issue/PR link, which weakens traceability in the release notes. Renaming to <pr>.misc.md clears this up.
Prompt for AI agents
Check if this issue is valid β if so, understand the root cause and fix it. At packages/reflex-components-react-player/news/+eng-10963-logging.misc.md, line 1:
<comment>This news fragment uses the orphan `+` prefix rather than the PR number. CONTRIBUTING.md says orphan fragments should be renamed after the PR opens; left as-is, towncrier materializes the changelog entry against a meaningless `+` reference instead of the issue/PR link, which weakens traceability in the release notes. Renaming to `<pr>.misc.md` clears this up.</comment>
<file context>
@@ -0,0 +1 @@
+Internal logging migrated from the legacy console helpers to standard python `logging` per-module loggers.
</file context>
| ) | ||
|
|
||
| console.warn( | ||
| logger.warning( |
There was a problem hiding this comment.
P3: The local from reflex_base.utils import console import in this function is now unused after the console.warn β logger.warning change β the import is the only remaining reference. Remove it to avoid a dead import (and any linter F401 warning).
Prompt for AI agents
Check if this issue is valid β if so, understand the root cause and fix it. At packages/reflex-base/src/reflex_base/event/__init__.py, line 1993:
<comment>The local `from reflex_base.utils import console` import in this function is now unused after the console.warn β logger.warning change β the import is the only remaining reference. Remove it to avoid a dead import (and any linter F401 warning).</comment>
<file context>
@@ -1987,7 +1990,7 @@ def _check_event_args_subclass_of_callback(
)
- console.warn(
+ logger.warning(
f"Event handler {key} expects ({expect_string}) -> () but got ({given_string}) -> (){as_annotated_in} instead. "
f"This may lead to unexpected behavior but is intentionally ignored for {key}."
</file context>
| assert mock_warn.call_count == 1 | ||
| mock_warn.assert_any_call( | ||
| "Dynamic route 'user/[user_id]/profile' does not have a 'loc' in sitemap configuration. Skipping." | ||
| warnings = [r for r in caplog.records if r.levelno == logging.WARNING] |
There was a problem hiding this comment.
P3: The rewritten dynamic-route and 404-route assertions count WARNING-level records across every logger via caplog (len(warnings) == 1), not just the sitemap plugin's logger. caplog captures records propagated to the root from all loggers, so an unrelated WARNING logged elsewhere while the test runs (a third-party warning, or a future warning added elsewhere in the sitemap flow) would make these exact-count assertions fail spuriously, and the assertion no longer isolates the behavior under test. Consider filtering by the plugin's logger name (e.g. r.name == "reflex_base.plugins.sitemap") or asserting on [r for r in caplog.records if "does not have a 'loc'" in r.getMessage()] so the tests stay robust and focused on the sitemap module's own warning.
Prompt for AI agents
Check if this issue is valid β if so, understand the root cause and fix it. At tests/units/plugins/test_sitemap.py, line 196:
<comment>The rewritten dynamic-route and 404-route assertions count WARNING-level records across every logger via caplog (`len(warnings) == 1`), not just the sitemap plugin's logger. caplog captures records propagated to the root from all loggers, so an unrelated WARNING logged elsewhere while the test runs (a third-party warning, or a future warning added elsewhere in the sitemap flow) would make these exact-count assertions fail spuriously, and the assertion no longer isolates the behavior under test. Consider filtering by the plugin's logger name (e.g. `r.name == "reflex_base.plugins.sitemap"`) or asserting on `[r for r in caplog.records if "does not have a 'loc'" in r.getMessage()]` so the tests stay robust and focused on the sitemap module's own warning.</comment>
<file context>
@@ -193,22 +193,23 @@ def mock_component():
- assert mock_warn.call_count == 1
- mock_warn.assert_any_call(
- "Dynamic route 'user/[user_id]/profile' does not have a 'loc' in sitemap configuration. Skipping."
+ warnings = [r for r in caplog.records if r.levelno == logging.WARNING]
+ assert len(warnings) == 1
+ assert (
</file context>
| warnings = [r for r in caplog.records if r.levelno == logging.WARNING] | |
| warnings = [ | |
| r for r in caplog.records | |
| if r.name == "reflex_base.plugins.sitemap" and r.levelno == logging.WARNING | |
| ] | |
| assert len(warnings) == 1 |
Replaces
console.debug/info/warn/errorcall sites with per-modulelogging.getLogger(__name__)loggers across reflex-base and the component packages (core, lucide, plotly, dataeditor, react-player). No behavior change beyond the new sink.Stack (ENG-10963)
#6863 β this β #6865 β #6866 β #6867.
Merge in order; each PR is based on the previous branch.