Skip to content

ENG-10963 refactor(log): migrate reflex-base and component packages to logging (2/5) - #6864

Open
FarhanAliRaza wants to merge 1 commit into
farhan/eng-10963-log-1-pipelinefrom
farhan/eng-10963-log-2-migrate-base
Open

ENG-10963 refactor(log): migrate reflex-base and component packages to logging (2/5)#6864
FarhanAliRaza wants to merge 1 commit into
farhan/eng-10963-log-1-pipelinefrom
farhan/eng-10963-log-2-migrate-base

Conversation

@FarhanAliRaza

@FarhanAliRaza FarhanAliRaza commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Replaces console.debug/info/warn/error call sites with per-module logging.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.

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.
@FarhanAliRaza
FarhanAliRaza requested a review from a team as a code owner August 10, 2026 20:36
@linear-code

linear-code Bot commented Aug 10, 2026

Copy link
Copy Markdown

ENG-10963

@greptile-apps

greptile-apps Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

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

  • Adds eager logging configuration when Config uses the default log level.
  • Preserves deduplication metadata for environment-driven config diagnostics.
  • Uses exception-aware logging for event-processor failures.
  • Updates affected tests to assert captured logging records and refreshes generated interface hashes.

Confidence Score: 5/5

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

Important Files Changed

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

@codspeed-hq

codspeed-hq Bot commented Aug 10, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

βœ… 26 untouched benchmarks
⏩ 8 skipped benchmarks1


Comparing farhan/eng-10963-log-2-migrate-base (666df42) with main (8e981e3)2

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

  2. No successful run was found on farhan/eng-10963-log-1-pipeline (3c9743d) during the generation of this report, so main (8e981e3) was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

πŸ’‘ 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`.")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge 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 πŸ‘Β / πŸ‘Ž.

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

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`.")

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.

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.

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.

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(

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.

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]

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.

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>
Suggested change
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

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.

1 participant