Skip to content

fix: expose deep Lucide imports through import_var - #7604

Merged
masenf merged 2 commits into
reflex-dev:mainfrom
sansynx:fix/lucide-import-metadata
Oct 23, 2026
Merged

masenf merged 2 commits into
reflex-dev:mainfrom
sansynx:fix/lucide-import-metadata

Conversation

@sansynx

@sansynx sansynx commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

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

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Changes

Declare static and dynamic Lucide module imports through import_var, so the public metadata describes the same deep module that the component imports. Let the base implementation collect and cache these imports together with imports required by props.

Preserve static default bindings, aliases and filename overrides, the named dynamic binding even when callers set is_default=True, and explicit package-root bundling. Add metadata and same-library prop-import regressions, and extend the existing dynamic-components app's browser coverage for initial and event-activated icons.

Closes #7111.

Validation

Tested on Windows with Python 3.12.10. Checks cover the patch committed as 0215ea166dce197170b5f7db23be0a0a0f7244cf.

  • Before the fix, six metadata/import-preservation cases failed.

  • uv run --python 3.12 --locked pytest tests/units/components/lucide/test_icon.py tests/units/compiler/test_dynamic_components_codegen.py -q --tb=short: 3,592 passed. Existing compiler tests cover explicit root bundling and specific deep-module registration.

  • All applicable pre-commit hooks passed, including Ruff, codespell, stub generation, Pyright, ty, and Biome. The first stub-generation run updated the required Lucide entry in pyi_hashes.json; the subsequent run passed.

  • With APP_HARNESS_HEADLESS=1 and PYTHONIOENCODING=utf-8, uv run --python 3.12 --locked pytest tests/integration/test_dynamic_components.py::test_dynamic_components -q -s -o faulthandler_timeout=120: 1 passed in 242.65 seconds. This extends the existing Selenium test, including its initial/reactive icons, repeated activation and counter events, with module-registry assertions and an explicit is_default=True caller. Its existing timeouts are unchanged; the 120-second diagnostic stack dump occurred during navigation, and pytest completed normally.

  • Rechecking the final six unit regressions against the original affected module produced 6 failures; restoring the fixed module produced 6 passes.

  • Cubic's caller-flag regression produced 2 failures and 2 passing controls before forcing the dynamic import to remain named. All four cases pass in the final unit run.

The browser follow-up initially failed during fixture setup. A diagnostic captured StaleElementReferenceException on the original token input while the current input was populated. The fixture now re-locates the input on each poll, retaining its existing 15-second timeout. The final passing run used no diagnostic plugin.

The initial uncaptured browser invocation failed while printing the server's Unicode arrow under Windows cp1252 output. Setting UTF-8 process output resolved that setup error. The full repository unit and integration matrices were not run locally; remote CI remains unverified until the workflows run.

An independent source review found no actionable correctness or security finding in this scoped patch. This is not a full repository security audit.

Implemented with assistance from OpenAI Codex. No human review or manual testing is claimed.

@sansynx
sansynx requested a review from a team as a code owner October 19, 2026 15:49
@greptile-apps

greptile-apps Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no blocking issues were found.

Summary

The PR exposes Lucide’s deep imports through import_var while preserving imports required by icon props.

  • Lucide icons expose deep module imports and keep prop imports.

No actionable new issues or repository-rule violations were found. No previous review threads were supplied. Tests were not run during this review.

Reviews (2) · Last reviewed commit: "fix: preserve named dynamic Lucide impor..." · Reviewed by Greptile

@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 5 files

Reply to a comment to ask cubic a question or push back. It learns from your replies.

View guided diff | Re-trigger cubic

Comment thread packages/reflex-components-lucide/src/reflex_components_lucide/icon.py Outdated
@masenf masenf added the bug Something isn't working label Oct 19, 2026 — with ChatGPT Codex Connector
@masenf masenf added the on deck PRs lined up to review / merge next label Oct 22, 2026
@codspeed

codspeed Bot commented Oct 23, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

1 warning

⚠️ 20 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

✅ 148 untouched benchmarks


Comparing sansynx:fix/lucide-import-metadata (0215ea1) with main (75a08e1)

Open in CodSpeed

@masenf
masenf merged commit a9b6613 into reflex-dev:main Oct 23, 2026
149 of 150 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working on deck PRs lined up to review / merge next

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Declare Lucide deep imports through import_var instead of _get_imports

2 participants