fix: re-enable telemetry and cover enable_tracking config#434
Merged
Conversation
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
📝 WalkthroughWalkthrough
Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #434 +/- ##
==========================================
+ Coverage 77.47% 78.01% +0.53%
==========================================
Files 35 35
Lines 4316 4317 +1
==========================================
+ Hits 3344 3368 +24
+ Misses 972 949 -23
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
dosubot
Bot
added
the
size:XS
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
comfy_cli/tracking.py (1)
55-55:⚠️ Potential issue | 🟠 MajorRefresh
distinct_idafter first-time enablement.
user_idis captured at import time, butinit_tracking(True)can create it later. On a first-run consent flow, the install event can still be sent withdistinct_id=None, breaking telemetry attribution now that tracking is re-enabled.🐛 Proposed fix
- mp.track(distinct_id=user_id, event_name=event_name, properties=properties) + current_user_id = config_manager.get(constants.CONFIG_KEY_USER_ID) + mp.track(distinct_id=current_user_id, event_name=event_name, properties=properties)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_cli/tracking.py` at line 55, The mp.track call uses the module-level user_id captured at import time which can be None if init_tracking(True) later creates the ID; before calling mp.track (the install/telemetry event), refresh/retrieve the current distinct id (e.g., call the function that returns/creates the user id or reassign user_id = get_user_id()/get_or_create_user_id()) and pass that refreshed value to mp.track so the event never gets sent with distinct_id=None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@comfy_cli/tracking.py`:
- Around line 48-50: The call to
config_manager.get_bool(constants.CONFIG_KEY_ENABLE_TRACKING) can raise
ValueError and must be guarded by the telemetry enable check; change the logic
in comfy_cli/tracking.py so that any ValueError when reading the tracking
boolean is caught and treated as False (disabled) inside the telemetry guard/try
block (i.e., wrap get_bool in try/except ValueError and default to False before
early-returning), and apply the same defensive pattern to the other
config_manager.get_bool usages referenced (the calls using
constants.CONFIG_KEY_ENABLE_TRACKING and the similar booleans at the other
locations) so malformed settings do not crash the CLI.
In `@tests/comfy_cli/test_tracking.py`:
- Around line 20-25: The tests patch tracking_mod.config_manager but not the
import-time derived global tracking_mod.user_id, so mp.track() can still use
ambient config; after patching tracking_mod.config_manager (and
tracking_mod.mp), explicitly reinitialize the import-time global by assigning
tracking_mod.user_id to the value from the test fixture config (e.g., set
tracking_mod.user_id = cfg.get("user_id") or the fixture’s user id) or call any
module init function that computes it; this ensures tracking_mod.mp.track() uses
the deterministic distinct_id for the test.
---
Outside diff comments:
In `@comfy_cli/tracking.py`:
- Line 55: The mp.track call uses the module-level user_id captured at import
time which can be None if init_tracking(True) later creates the ID; before
calling mp.track (the install/telemetry event), refresh/retrieve the current
distinct id (e.g., call the function that returns/creates the user id or
reassign user_id = get_user_id()/get_or_create_user_id()) and pass that
refreshed value to mp.track so the event never gets sent with distinct_id=None.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: edfc06be-079a-4960-a9e2-585f6490382d
📒 Files selected for processing (2)
comfy_cli/tracking.pytests/comfy_cli/test_tracking.py
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/comfy_cli/test_tracking.py`:
- Around line 87-90: The test test_enable_generates_user_id should also assert
that the runtime tracking uses the newly persisted ID: after calling
tracking_module.init_tracking(True) and verifying
config_manager.get(constants.CONFIG_KEY_USER_ID) is not None, also assert
tracking_module.user_id ==
tracking_module.config_manager.get(constants.CONFIG_KEY_USER_ID) and that
subsequent calls to track_event(...) send that user_id as distinct_id (i.e.,
ensure track_event uses the module-global user_id). If that fails, update
comfy_cli/tracking.py:init_tracking to assign the persisted ID into the
module-global tracking_module.user_id so runtime tracking and track_event() use
the generated ID.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 16460d4e-6353-42e0-a1c6-2bff5adf1704
📒 Files selected for processing (1)
tests/comfy_cli/test_tracking.py
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
comfy_cli/tracking.py (1)
48-57:⚠️ Potential issue | 🟠 MajorGuard malformed tracking config before telemetry can trip the CLI.
Line 48 can raise
ValueErrorfor an invalid boolean string before the telemetrytry/except, so a bad config value can crash normal commands. Put the lookup inside the guard; if it croaks, tracking should stay off — no impish config string should sink the ship.🛡️ Proposed fix
- enable_tracking = config_manager.get_bool(constants.CONFIG_KEY_ENABLE_TRACKING) - if not enable_tracking: - return - try: + enable_tracking = config_manager.get_bool(constants.CONFIG_KEY_ENABLE_TRACKING) + if not enable_tracking: + return + properties["cli_version"] = cli_version properties["tracing_id"] = tracing_id mp.track(distinct_id=user_id, event_name=event_name, properties=properties)Verification:
#!/bin/bash # Description: Confirm invalid configparser booleans raise ValueError. python - <<'PY' import configparser cfg = configparser.ConfigParser() cfg["DEFAULT"]["enable_tracking"] = "maybe" try: cfg.getboolean("DEFAULT", "enable_tracking") except Exception as exc: print(type(exc).__name__) print(exc) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_cli/tracking.py` around lines 48 - 57, The call to config_manager.get_bool(constants.CONFIG_KEY_ENABLE_TRACKING) can raise on malformed config and must not run before the telemetry guard; change the code so you attempt the get_bool inside a safe guard (e.g., wrap the get_bool call in a try/except that catches ValueError/Exception and treats enable_tracking as False, optionally logging a warning), then only proceed to set properties["cli_version"], properties["tracing_id"] and call mp.track(...) inside the existing try block when enable_tracking is True; reference symbols: enable_tracking, config_manager.get_bool, constants.CONFIG_KEY_ENABLE_TRACKING, mp.track, properties, cli_version, tracing_id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/comfy_cli/test_tracking.py`:
- Around line 59-62: The test test_swallows_mixpanel_errors should assert that
Mixpanel was actually called before confirming swallow behavior: after setting
tracking_module.mp.track.side_effect = RuntimeError("boom") and calling
tracking_module.track_event("some_event"), add an assertion that
tracking_module.mp.track was called (e.g., assert
tracking_module.mp.track.called or assert_called_once_with appropriate args) so
the RuntimeError path is exercised; ensure the assertion references
tracking_module.mp.track and tracking_module.track_event and preserves the
CONFIG_KEY_ENABLE_TRACKING setup via constants.CONFIG_KEY_ENABLE_TRACKING.
---
Duplicate comments:
In `@comfy_cli/tracking.py`:
- Around line 48-57: The call to
config_manager.get_bool(constants.CONFIG_KEY_ENABLE_TRACKING) can raise on
malformed config and must not run before the telemetry guard; change the code so
you attempt the get_bool inside a safe guard (e.g., wrap the get_bool call in a
try/except that catches ValueError/Exception and treats enable_tracking as
False, optionally logging a warning), then only proceed to set
properties["cli_version"], properties["tracing_id"] and call mp.track(...)
inside the existing try block when enable_tracking is True; reference symbols:
enable_tracking, config_manager.get_bool, constants.CONFIG_KEY_ENABLE_TRACKING,
mp.track, properties, cli_version, tracing_id.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 91752dc7-ac6e-472e-8bac-43a7c47ca51e
📒 Files selected for processing (2)
comfy_cli/tracking.pytests/comfy_cli/test_tracking.py
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/comfy_cli/test_tracking.py`:
- Around line 88-96: Add a new test (e.g.,
test_enable_preserves_existing_user_id) that exercises
tracking_module.init_tracking to ensure an existing user_id is preserved when
re-enabling: call tracking_module.init_tracking(True) to generate and read
first_user_id from
tracking_module.config_manager.get(constants.CONFIG_KEY_USER_ID), call
init_tracking(False) then init_tracking(True) again, and assert
tracking_module.config_manager.get(constants.CONFIG_KEY_USER_ID) and
tracking_module.user_id still equal first_user_id (i.e., no new ID is
generated).
- Around line 78-83: Add a short inline comment in the
test_enable_is_respected_by_track_event test explaining why
tracking_module.mp.track.reset_mock() is called: note that init_tracking(True)
triggers an initial "install" (or similar) tracking call which we intentionally
clear so the subsequent tracking_module.track_event("some_event") assertion only
validates the event under test; reference the reset_mock call and the
init_tracking/track_event/mp.track symbols in the comment for clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed6edd4d-c5eb-4840-a972-e22c6465c81b
📒 Files selected for processing (1)
tests/comfy_cli/test_tracking.py
Comment on lines
+78
to
+83
| def test_enable_is_respected_by_track_event(self, tracking_module): | ||
| tracking_module.init_tracking(True) | ||
| tracking_module.mp.track.reset_mock() | ||
| tracking_module.track_event("some_event") | ||
| tracking_module.mp.track.assert_called_once() | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a comment to explain reset_mock() — it's a mock-ness of mercy!
Line 80 resets the mock counter after init_tracking(True), presumably because init fires an "install" event that shouldn't count toward the subsequent track_event assertion. A brief comment would help future maintainers understand why this reset is necessary.
📝 Proposed clarity improvement
def test_enable_is_respected_by_track_event(self, tracking_module):
tracking_module.init_tracking(True)
+ # init_tracking fires an "install" event; reset to test subsequent track_event calls
tracking_module.mp.track.reset_mock()
tracking_module.track_event("some_event")
tracking_module.mp.track.assert_called_once()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_enable_is_respected_by_track_event(self, tracking_module): | |
| tracking_module.init_tracking(True) | |
| tracking_module.mp.track.reset_mock() | |
| tracking_module.track_event("some_event") | |
| tracking_module.mp.track.assert_called_once() | |
| def test_enable_is_respected_by_track_event(self, tracking_module): | |
| tracking_module.init_tracking(True) | |
| # init_tracking fires an "install" event; reset to test subsequent track_event calls | |
| tracking_module.mp.track.reset_mock() | |
| tracking_module.track_event("some_event") | |
| tracking_module.mp.track.assert_called_once() |
🧰 Tools
🪛 Pylint (4.0.5)
[convention] 78-78: Missing function or method docstring
(C0116)
[warning] 78-78: Redefining name 'tracking_module' from outer scope (line 13)
(W0621)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/comfy_cli/test_tracking.py` around lines 78 - 83, Add a short inline
comment in the test_enable_is_respected_by_track_event test explaining why
tracking_module.mp.track.reset_mock() is called: note that init_tracking(True)
triggers an initial "install" (or similar) tracking call which we intentionally
clear so the subsequent tracking_module.track_event("some_event") assertion only
validates the event under test; reference the reset_mock call and the
init_tracking/track_event/mp.track symbols in the comment for clarity.
Comment on lines
+88
to
+96
| def test_enable_generates_user_id(self, tracking_module): | ||
| assert tracking_module.config_manager.get(constants.CONFIG_KEY_USER_ID) is None | ||
| tracking_module.init_tracking(True) | ||
| generated_user_id = tracking_module.config_manager.get(constants.CONFIG_KEY_USER_ID) | ||
| assert generated_user_id is not None | ||
| assert tracking_module.user_id == generated_user_id | ||
| _, kwargs = tracking_module.mp.track.call_args | ||
| assert kwargs["distinct_id"] == generated_user_id | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a test for re-enabling with existing user_id.
The current test validates first-time user_id generation, but doesn't cover the case where a user disables then re-enables tracking. Consider adding a test that verifies init_tracking(True) preserves an existing user_id rather than generating a new one, ensuring tracking consistency across enable/disable cycles.
🧪 Proposed additional test
def test_enable_preserves_existing_user_id(self, tracking_module):
# First enable generates ID
tracking_module.init_tracking(True)
first_user_id = tracking_module.config_manager.get(constants.CONFIG_KEY_USER_ID)
# Disable then re-enable
tracking_module.init_tracking(False)
tracking_module.init_tracking(True)
# Should preserve the same user_id
assert tracking_module.config_manager.get(constants.CONFIG_KEY_USER_ID) == first_user_id
assert tracking_module.user_id == first_user_id🧰 Tools
🪛 Pylint (4.0.5)
[convention] 88-88: Missing function or method docstring
(C0116)
[warning] 88-88: Redefining name 'tracking_module' from outer scope (line 13)
(W0621)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/comfy_cli/test_tracking.py` around lines 88 - 96, Add a new test (e.g.,
test_enable_preserves_existing_user_id) that exercises
tracking_module.init_tracking to ensure an existing user_id is preserved when
re-enabling: call tracking_module.init_tracking(True) to generate and read
first_user_id from
tracking_module.config_manager.get(constants.CONFIG_KEY_USER_ID), call
init_tracking(False) then init_tracking(True) again, and assert
tracking_module.config_manager.get(constants.CONFIG_KEY_USER_ID) and
tracking_module.user_id still equal first_user_id (i.e., no new ID is
generated).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR #336 temporarily disabled Mixpanel telemetry
This PR restores the config lookup in
comfy_cli/tracking.pyby removing the hardcoded False and uncommenting the call.