Skip to content

migrate all time env vars to timedelta - #7138

Open
benedikt-bartscher wants to merge 11 commits into
reflex-dev:mainfrom
benedikt-bartscher:migrate-envvar-timedelta
Open

benedikt-bartscher wants to merge 11 commits into
reflex-dev:mainfrom
benedikt-bartscher:migrate-envvar-timedelta

Conversation

@benedikt-bartscher

@benedikt-bartscher benedikt-bartscher commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

follow-up of #7131

Review in cubic

@greptile-apps

greptile-apps Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge. One warning-location bug is non-blocking.

Findings

  1. P2 Generated frames look like users

Summary

Time-based environment variables now use timedelta values with unit suffixes, while old names keep working during migration. Deprecation warnings also skip generated frames so they point to real user code.

  • Duration settings accept values such as 2m, while bare numbers still mean seconds.
  • Old millisecond and second-based names warn once and suggest their unit-suffixed replacements.
  • App, database, websocket, browser, disk-state, and Redis code convert durations for their existing callers.
  • Deprecation locations skip <string> and <frozen ...> frames but keep interactive user locations.

Reviews (7) · Last reviewed commit: "Fix two review findings on the duration ..."

Comment thread reflex/istate/manager/redis.py Outdated
Comment thread reflex/app.py
@codspeed-hq

codspeed-hq Bot commented Sep 14, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 40 untouched benchmarks
⏩ 8 skipped benchmarks1


Comparing benedikt-bartscher:migrate-envvar-timedelta (58fc137) with main (2f63cb3)

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.

Comment thread reflex/istate/manager/redis.py Outdated
@benedikt-bartscher
benedikt-bartscher marked this pull request as ready for review September 14, 2026 18:45
@benedikt-bartscher
benedikt-bartscher requested a review from a team as a code owner September 14, 2026 18:45

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

No issues found across 13 files

Re-trigger cubic

Comment thread packages/reflex-base/src/reflex_base/environment.py Outdated
masenf
masenf previously approved these changes Sep 14, 2026
masenf and others added 2 commits September 14, 2026 15:17
Review follow-ups for the timedelta migration of the duration env vars:

- Treat a blank value as unset, matching EnvVar semantics, so a templated
  `REFLEX_STATE_MANAGER_DISK_DEBOUNCE=` no longer shadows the old name and a
  blank old name no longer warns.
- Name the exact replacement in the deprecation message
  (`REFLEX_AUTO_RELOAD_COOLDOWN=5000ms`), since renaming the variable without
  its unit would silently read the value as seconds.
- Warn once per process rather than once per call site, and read the
  auto-reload cooldown during app setup so its deprecation shows at startup
  instead of on the first matching frontend error.
- Reject a negative opportunistic lock hold time instead of clamping it to
  one millisecond.
- Skip bracketed pseudo-filenames when locating the user frame for a
  deprecation. A code object with no source file carries `<string>` (exec, a
  generated dataclass `__init__`) or `<frozen importlib._bootstrap>` rather
  than a path, and both were reported as the deprecation's location.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gg8XKw3GThDkGC2L4y5ZSH

@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 10 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread packages/reflex-base/src/reflex_base/environment.py Outdated
Comment thread packages/reflex-base/src/reflex_base/utils/log.py Outdated
- Build the suggested replacement from the parsed value rather than the raw
  text. A bare int or float accepts forms the duration parser rejects, so
  `REFLEX_STATE_MANAGER_DISK_DEBOUNCE_SECONDS=.5` suggested `.5s`, which fails
  to parse. `1_000` and `1e3` had the same problem.
- Skip only `<string>` and `<frozen ...>` when locating the user frame for a
  deprecation, instead of every bracketed name. `<stdin>` and an
  `<ipython-input-N-...>` cell are real user call sites, and swallowing them
  pushed the reported location up into the interpreter's own frames.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gg8XKw3GThDkGC2L4y5ZSH
Comment on lines +363 to +364
if filename == "<string>" or filename.startswith("<frozen "):
return True

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 This skips only <string> and <frozen ...>. A generated frame such as <attrs generated init reflex.Thing> reaches Path.resolve(), so _get_first_non_framework_frame stops there and reports a fake file. Keep interactive names as user frames, but skip other generated bracketed names. Apply the same fix in log.py.

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

1 issue found across 10 files (changes from recent commits).

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-base/src/reflex_base/utils/console.py">

<violation number="1" location="packages/reflex-base/src/reflex_base/utils/console.py:363">
P2: Skip generated bracketed filenames such as `<attrs generated init reflex.Thing>` before calling `Path.resolve()`, and apply the same predicate in `log.py`. Otherwise deprecation lookup reports the generated pseudo-frame as a fake user file.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

# would depend on where the app was started from. Other bracketed names are
# left alone on purpose: `<stdin>` and an `<ipython-input-N-...>` cell are
# exactly where an interactive user would look for their own call.
if filename == "<string>" or filename.startswith("<frozen "):

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: Skip generated bracketed filenames such as <attrs generated init reflex.Thing> before calling Path.resolve(), and apply the same predicate in log.py. Otherwise deprecation lookup reports the generated pseudo-frame as a fake user file.

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/utils/console.py, line 363:

<comment>Skip generated bracketed filenames such as `<attrs generated init reflex.Thing>` before calling `Path.resolve()`, and apply the same predicate in `log.py`. Otherwise deprecation lookup reports the generated pseudo-frame as a fake user file.</comment>

<file context>
@@ -353,6 +353,15 @@ def _is_framework_filename(filename: str) -> bool:
+    # would depend on where the app was started from. Other bracketed names are
+    # left alone on purpose: `<stdin>` and an `<ipython-input-N-...>` cell are
+    # exactly where an interactive user would look for their own call.
+    if filename == "<string>" or filename.startswith("<frozen "):
+        return True
     frame_path = Path(filename).resolve()
</file context>

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.

3 participants