feat: add per-account statistics dashboard#61
Merged
Conversation
Collaborator
Author
|
I'd like to get your opinion on this feature. I made it the way I envisioned it, but I don't know if it's what you were expecting. |
safarsin
requested review from
Copilot and
safarsin
and removed request for
safarsin
Owner
|
Hi @JeromeM, this is absolutely amazing. It looks even better than I imagined. I did notice one small UI detail: the new STATS card pushes the ACTIVITY block down, making it cut off at the bottom. We probably just need to increase the default main window height a bit so everything fits. 
I'm a bit short on time right now, but I'll do a proper code review and test everything as soon as I have more free time. Amazing work as always, thanks! |
Collaborator
Author
|
I don't know if it's a good or not to have the activity still on the main window like this. Actually when you click on history you have the full activity log. |
Collaborator
Author
|
Done ! |
Owner
|
Let's keep the simple Activity status (like "Search 1/30...") on the main window. It's super important for real-time feedback so users know the script is actually running and for debugging. If you think that the window is getting too tall, we can just trim a bit from the header and footer. However, having the full History table as a separate window was actually much better for UX. Embedding it in the main window makes it too cramped and hard to scroll. Let's go back to having it separate, like it was. 
No rush at all, take your time, and thanks for your patience. |
Owner
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Statistics dashboard with per-account ChangesStatistics Dashboard
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dailytasks/runner.py (1)
392-410: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPublish partial
last_totalsas each section finishes.
self.last_totalsis only updated after the whole loop. If an earlier section completes cards and a later section throws into the outerexcept,src/api.py:2152-2160/src/api.py:2210-2222will readnewly=0and under-record the session. Updateself.last_totalsinside the loop after each section result is merged.Proposed fix
totals = {"already": 0, "newly": 0, "final": 0, "total": 0, "attempted": 0} for section_name, selector in SECTIONS: if stop_event is not None and stop_event.is_set(): self._log("Stop requested — skipping remaining sections.") break section_result = self._process_section( driver, human, section_name, selector, main_tab, stop_event=stop_event, ) for k in totals: totals[k] += section_result[k] + self.last_totals = dict(totals) - # Expose the run's aggregated counts for the stats layer. - self.last_totals = dict(totals) + # Expose the run's aggregated counts for the stats layer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dailytasks/runner.py` around lines 392 - 410, The aggregated run totals in the runner are only written to self.last_totals after all sections finish, so a later failure can hide progress from the API stats path. Update the DailyTasks runner logic in the section-processing loop to copy the merged totals into self.last_totals after each successful _process_section call, while keeping the final assignment at the end as a fallback. Use the existing symbols self.last_totals, _process_section, and the SECTIONS loop in runner.py so src/api.py can read partial progress even if a later section raises.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gui/dashboard.css`:
- Around line 157-160: The font-family declaration in .diag-list is using a
quoted Consolas entry that Stylelint rejects. Update the .diag-list rule to use
the unquoted Consolas font name while keeping the rest of the font stack
unchanged.
In `@gui/index.html`:
- Around line 116-117: The stats card is using a generic section as a button,
but it is not keyboard-activatable. Update the stats card markup in the
stats_card element so it supports Enter/Space activation, either by switching to
a real button or by adding the appropriate keyboard handler alongside
show_stats(). Make sure the existing onclick behavior stays intact and the
control remains accessible to keyboard users.
In `@gui/script.js`:
- Around line 1113-1114: Reload the inline history whenever the active account
changes by updating the account-switch path in refresh_stats_ui() or the nearby
selection-change handler so it also repopulates the history view, not just the
compact stats. Make sure the logic that currently lives in toggle_history_view()
is reused or extracted so history_body is refreshed for the newly selected
account when _historyShown is already open.
In `@gui/styles.css`:
- Around line 800-805: The `.history-inline-table td` rule uses deprecated
`word-break: break-word`, which triggers the Stylelint failure. Update this
cell-wrapping behavior by replacing it with the appropriate `overflow-wrap`
styling in the same selector so the history table text still wraps correctly
without using the deprecated value.
In `@src/api.py`:
- Around line 322-329: The warmup balance refresh in load_driver_in_background()
/ _refresh_balance_on_launch() is using mutable self.stats and
self.account_meta, so a later account switch can save the wrong profile’s
balance. Capture the account identity/state when the warmup driver is created,
pass that immutable snapshot into _refresh_balance_on_launch(), and only persist
to stats.json if it still matches the account that opened the driver. Make the
same safeguard in the related refresh path around the same logic so the balance
update always stays pinned to the original account.
- Around line 1737-1748: The busy check in refresh_balance is non-atomic, so
concurrent callers can both pass before either owns the EdgeProfile. Add a
dedicated lock around the full refresh path in refresh_balance, starting before
setup_driver and held until the driver is cleaned up, so only one profile user
can open the driver at a time. Keep the existing self._run_lock.locked() and
self.is_driver_loading checks, but make the refresh itself serialize through the
new lock to prevent races with Start and other refreshes.
In `@src/stats/manager.py`:
- Around line 83-88: The balance scrape in the points selector logic is
rejecting valid zero values because it only returns when the extracted value is
greater than 0. Update the validation in the relevant balance-selection block so
a numeric 0 is treated as a successful scrape, while still excluding
null/undefined and invalid values. Keep the return shape unchanged in the same
flow that produces the object with value, via, candidates, url, and title.
---
Outside diff comments:
In `@src/dailytasks/runner.py`:
- Around line 392-410: The aggregated run totals in the runner are only written
to self.last_totals after all sections finish, so a later failure can hide
progress from the API stats path. Update the DailyTasks runner logic in the
section-processing loop to copy the merged totals into self.last_totals after
each successful _process_section call, while keeping the final assignment at the
end as a fallback. Use the existing symbols self.last_totals, _process_section,
and the SECTIONS loop in runner.py so src/api.py can read partial progress even
if a later section raises.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d1fe344-0b77-494a-b05f-608edfc58fce
📒 Files selected for processing (14)
AutoRewarder.pyAutoRewarder_CLI.pyREADME.mdgui/dashboard.cssgui/dashboard.htmlgui/index.htmlgui/script.jsgui/styles.csssrc/api.pysrc/config.pysrc/dailytasks/runner.pysrc/search/engine.pysrc/stats/__init__.pysrc/stats/manager.py
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/api.py (2)
1773-1783: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftUse the same profile lock for runs and refreshes.
refresh_balance()now serializes other manual refreshes, butmain()and the startup warmup do not acquire_balance_lock. A Start action can still pass after Line 1776 and open the same Edge profile while the refresh owns it.Consider a single profile-driver lock used by every
setup_driver()path, or make Start/warmup reject while_balance_lockis held.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api.py` around lines 1773 - 1783, The locking in refresh_balance() is only protecting manual refreshes, while main() and the startup warmup path still call setup_driver() without respecting _balance_lock, so a Start can race and open the same Edge profile. Update the shared driver-setup flow in main(), the warmup path, and refresh_balance() to use one common profile lock (or explicitly check _balance_lock before starting) so every driver acquisition on EdgeProfile is serialized through the same mechanism.
358-364: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCapture the warmup account before opening the driver.
_refresh_balance_on_launch()snapshotsself.stats/self.account_metaonly aftersetup_driver()has already opened a profile. If the selected account changes while the warmup driver is being created, the old profile’s balance can still be written through the new account’s managers.Proposed direction
- warmup_driver = self.driver_manager.setup_driver(headless=True) + warmup_account_id = self.account_manager.current_id() + warmup_stats = self.stats + warmup_meta = self.account_meta + warmup_driver_manager = self.driver_manager + warmup_driver = warmup_driver_manager.setup_driver(headless=True) try: @@ - self._refresh_balance_on_launch(warmup_driver) + self._refresh_balance_on_launch( + warmup_driver, + account_id=warmup_account_id, + stats=warmup_stats, + meta=warmup_meta, + )Also applies to: 1721-1727
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api.py` around lines 358 - 364, Capture the selected warmup account state before calling setup_driver() in API._refresh_balance_on_launch flow, because snapshotting self.stats and self.account_meta after the driver is already opened can mix an old profile with a newly selected account. Store the account identifiers/managers up front, then pass or use that captured state inside _refresh_balance_on_launch() so the warmup balance refresh always writes back to the same account that was selected when launch started. Apply the same adjustment in the duplicated warmup path referenced by the other call site.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/api.py`:
- Around line 1773-1783: The locking in refresh_balance() is only protecting
manual refreshes, while main() and the startup warmup path still call
setup_driver() without respecting _balance_lock, so a Start can race and open
the same Edge profile. Update the shared driver-setup flow in main(), the warmup
path, and refresh_balance() to use one common profile lock (or explicitly check
_balance_lock before starting) so every driver acquisition on EdgeProfile is
serialized through the same mechanism.
- Around line 358-364: Capture the selected warmup account state before calling
setup_driver() in API._refresh_balance_on_launch flow, because snapshotting
self.stats and self.account_meta after the driver is already opened can mix an
old profile with a newly selected account. Store the account
identifiers/managers up front, then pass or use that captured state inside
_refresh_balance_on_launch() so the warmup balance refresh always writes back to
the same account that was selected when launch started. Apply the same
adjustment in the duplicated warmup path referenced by the other call site.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c94001c6-1053-4ffe-b2a8-539b0a6e893a
📒 Files selected for processing (9)
AutoRewarder.pygui/dashboard.cssgui/dashboard.htmlgui/index.htmlgui/script.jsgui/styles.csssrc/api.pysrc/dailytasks/runner.pysrc/stats/manager.py
💤 Files with no reviewable changes (2)
- gui/styles.css
- src/dailytasks/runner.py
🚧 Files skipped from review as they are similar to previous changes (4)
- AutoRewarder.py
- gui/dashboard.css
- gui/dashboard.html
- src/stats/manager.py
Collaborator
Author
|
I've done the update. You can check if everything is ok. |
Introduce a statistics feature that tracks Microsoft Rewards points and activity per account, surfaced both as a compact card in the main window and a dedicated dashboard window. Data layer: - New src/stats package with StatsManager, persisting a per-account stats.json (atomic writes + corruption recovery, mirroring HistoryManager). - Track lifetime counters (PC/mobile searches, daily cards, runs) and an estimated points figure, plus a rolling window of recent runs. - Scrape the real Rewards balance from rewards.bing.com via a robust, balance-scoped selector (reads the counter's aria-label), with a diagnostic fallback. The scraped balance is the source of truth for the total; the delta between runs gives points earned per session. - add stats_path() helper and document stats.json in the storage layout. Collection points: - SearchEngine.perform_searches now returns the number of successful searches. - DailySet exposes last_totals so newly completed cards can be recorded. - AutoRewarderAPI accumulates per-run counts and the scraped balance, then records a session on completion (works for both GUI and headless CLI runs). - Refresh the balance on every launch via the existing warmup driver; animate the card only on first capture, otherwise update silently in the background. UI: - Compact, clickable stats card (total points + points earned last run) on the main window, opening the dashboard. - New dashboard window (dashboard.html/css): headline totals, lifetime activity metrics, a per-day stacked bar chart split into searches vs daily points, and a multi-account recap. Includes an on-demand "Refresh balance" action with a themed progress state and an in-place diagnostic box on failure. - Dashboard docks to the right edge of the main window when opened. Fixes: - Closing the main window now also destroys open secondary windows (History, Statistics) so the app actually quits instead of lingering in the background. Docs: - README: mark the statistics dashboard roadmap item done, list it under features, and add the stats module and dashboard files to the project tree.
…story
Refine the statistics feature based on real-world usage.
Data model:
- Store activity as per-day aggregates ("YYYY-MM-DD" -> pc/mobile/cards/runs)
instead of individual run records, capped at 90 days. Advanced scheduling
fires one session per query, so the old per-run rolling window filled up
within a single day and evicted every earlier day from the timeline.
- Migrate existing stats.json: legacy per-run records are folded into per-day
aggregates on load, preserving whatever history the old window still held.
Activity chart:
- One stacked bar per day with three colour-coded segments — PC, Mobile and
Daily points — instead of a single search/daily split.
- Fixed-width columns packed from the left; only the most recent days that fit
the current width are shown, with the oldest dropping off the left edge.
- Recompute the visible day count from the real element width via a
ResizeObserver, fixing a first-paint mismeasurement that collapsed the chart
to a single day.
Main window:
- Show the execution history inline in the Activity card: the button now
toggles the card between the live log and the history table in place, instead
of opening a separate window.
- Enlarge the window (640x680 -> 680x900) so the inline history has room.
Misc:
- Drop the internal "(via <selector>)" detail from the user-facing
"Points balance scraped" log line; it is kept only in failure diagnostics.
Shared chart legend and segment colours move to styles.css (loaded by both the
main window and the dashboard) to avoid duplication.
Address UX feedback on the statistics feature. - Revert the inline execution history: the Activity card again shows only the live log (important real-time "Search 1/30..." feedback), and "View history" reopens the dedicated window, which scrolls and reads far better than the cramped in-card table. - Make the Statistics and History windows single-instance: reopening reuses and reloads the existing window (fresh data, brought to front) instead of stacking another one. Repeated opens could otherwise pile up windows and leave the webview backend rendering a blank one. - Add bottom padding inside the dashboard's content container so the "All accounts" table no longer sits flush against the window edge (a scroll container's own padding-bottom is dropped by some webview engines). - Stop printing internal "[DIAG]" lines (balance scrape selectors / page, daily-set icon classes) to the activity feed; the raw scrape diagnostic is still surfaced in the dashboard only when a manual balance refresh fails. - Enlarge the main window (640x680 -> 680x840) so the activity log has room.
…cases - Pin balance scrapes to the account that opened the driver: both the on-launch warmup refresh and the dashboard "Refresh balance" capture the account id + StatsManager up front and only persist if the active account is unchanged, so a mid-scrape account switch can't write one profile's balance into another's. - Serialize ad-hoc balance refreshes through a dedicated lock held across the whole driver lifecycle, so concurrent refreshes can't open two drivers on the same Edge profile (the prior busy check was non-atomic). - Accept a real balance of 0 in the scrape selector (was rejecting >= 0 as "not found"), so a brand-new account's zero balance is captured correctly. - Make the stats card keyboard-activatable (Enter / Space) in addition to click, since it exposes role="button". - Drop the quotes around the single-token Consolas font name in the dashboard diagnostic list to satisfy Stylelint. - Add bottom padding inside the dashboard content container so the "All accounts" table isn't flush against the window edge.
JeromeM
force-pushed
the
feature/stats_dashboard
branch
from
30537de to
3aba383
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/api.py (2)
358-364: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCapture the warmup account before opening the driver.
Line 364 still lets
_refresh_balance_on_launch()captureself.stats/current_idaftersetup_driver()returns. If the user switches accounts while Line 358 is opening the old profile, the old profile’s balance can be saved to the newly selected account.Proposed direction
+ warmup_account_id = self.account_manager.current_id() + warmup_stats = self.stats + warmup_meta = self.account_meta + warmup_driver_manager = self.driver_manager self.is_driver_loading = True try: - warmup_driver = self.driver_manager.setup_driver(headless=True) + warmup_driver = warmup_driver_manager.setup_driver(headless=True) try: @@ - self._refresh_balance_on_launch(warmup_driver) + self._refresh_balance_on_launch( + warmup_driver, + expected_account_id=warmup_account_id, + stats=warmup_stats, + meta=warmup_meta, + )def _refresh_balance_on_launch( self, driver, expected_account_id=None, stats=None, meta=None, ): stats = stats if stats is not None else self.stats meta = meta if meta is not None else self.account_meta account_id = expected_account_id or self.account_manager.current_id() ...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api.py` around lines 358 - 364, Capture the selected account context before calling setup_driver() in the launch path, because _refresh_balance_on_launch() currently reads self.stats and current_id too late and can write the warmup profile’s balance onto a newly selected account. Update the launch flow in the method that calls setup_driver(headless=True) to pass the pre-opened account identifiers/state into _refresh_balance_on_launch(), and extend _refresh_balance_on_launch() to accept explicit expected_account_id, stats, and meta parameters while defaulting to self.account_manager.current_id(), self.stats, and self.account_meta only when not provided.
1779-1783: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse the refresh lock for Start runs too.
Line 1782 only serializes refresh-vs-refresh.
main()still opens profile drivers under_run_lockalone, so Start can race with a manual refresh and both can use the same Edge profile.Proposed direction
if not self._run_lock.acquire(blocking=False): self.log("[WARNING] A run is already in progress.") return + if not self._balance_lock.acquire(blocking=False): + self.log("[WARNING] Driver is busy. Try again shortly.") + self._run_lock.release() + if self._webview_window: + self._webview_window.evaluate_js("enable_start_button()") + return # Reset stop flag before each run so a previous Stop doesn't carry over. @@ finally: @@ self._run_lock.release() + self._balance_lock.release()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api.py` around lines 1779 - 1783, The profile claim in the refresh path only guards refresh-vs-refresh, so Start runs can still race with a manual refresh and open the same Edge profile. Update the locking flow in src/api.py so the Start path in main() also acquires the same refresh/profile lock used around the balance refresh claim, and make sure the combined lock ordering still prevents concurrent driver creation for the same profile. Reference the existing _balance_lock and _run_lock handling, and align the Start-run entrypoint with the same atomic profile access used by the refresh code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AutoRewarder.py`:
- Around line 70-73: The secondary-window teardown in AutoRewarder.py is
swallowing destroy() failures in the cleanup path, which hides important
diagnostics. Update the try/except around other.destroy() to surface the
exception instead of passing it silently, using the cleanup logic in the
secondary-window teardown block to log or re-raise the failure so the app can
report what went wrong.
In `@gui/script.js`:
- Around line 417-440: The refresh_stats_ui() callback is applying asynchronous
get_stats() results even when they belong to a previously selected account,
which can overwrite the compact stats card with stale data. Use the returned
stats.account.id from pywebview.api.get_stats() to verify the response still
matches the currently active account before mutating stat_total, stat_session,
or stat_total_label, and ignore outdated responses when the account has changed.
In `@src/stats/__init__.py`:
- Around line 11-17: The __all__ list in the module-level export block is not
sorted, which Ruff flags with RUF022. Reorder the entries in __all__ in a
consistent sorted order, keeping the same exported symbols (StatsManager,
scrape_points_balance, scrape_points_balance_debug, POINTS_PER_SEARCH,
POINTS_PER_CARD) so the lint rule passes without changing behavior.
In `@src/stats/manager.py`:
- Around line 61-68: The selector fallback list in the balance lookup is too
broad because unscoped `.pointsValue` entries can match
daily-points/streak/referral tiles and be persisted as the balance. Update the
selector list used in the stats manager’s balance scraping logic to keep only
selectors scoped to the rewards balance component (for example the existing
`mee-rewards-user-status-banner-balance` and `#balanceToolTipDiv` variants) and
remove the generic `.pointsValue` fallbacks from the selector array.
---
Duplicate comments:
In `@src/api.py`:
- Around line 358-364: Capture the selected account context before calling
setup_driver() in the launch path, because _refresh_balance_on_launch()
currently reads self.stats and current_id too late and can write the warmup
profile’s balance onto a newly selected account. Update the launch flow in the
method that calls setup_driver(headless=True) to pass the pre-opened account
identifiers/state into _refresh_balance_on_launch(), and extend
_refresh_balance_on_launch() to accept explicit expected_account_id, stats, and
meta parameters while defaulting to self.account_manager.current_id(),
self.stats, and self.account_meta only when not provided.
- Around line 1779-1783: The profile claim in the refresh path only guards
refresh-vs-refresh, so Start runs can still race with a manual refresh and open
the same Edge profile. Update the locking flow in src/api.py so the Start path
in main() also acquires the same refresh/profile lock used around the balance
refresh claim, and make sure the combined lock ordering still prevents
concurrent driver creation for the same profile. Reference the existing
_balance_lock and _run_lock handling, and align the Start-run entrypoint with
the same atomic profile access used by the refresh code.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0609c23-d52e-4d1c-876f-eceeb4288fe3
📒 Files selected for processing (14)
AutoRewarder.pyAutoRewarder_CLI.pyREADME.mdgui/dashboard.cssgui/dashboard.htmlgui/index.htmlgui/script.jsgui/styles.csssrc/api.pysrc/config.pysrc/dailytasks/runner.pysrc/search/engine.pysrc/stats/__init__.pysrc/stats/manager.py
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- src/config.py
- gui/index.html
- AutoRewarder_CLI.py
- src/dailytasks/runner.py
- gui/styles.css
- gui/dashboard.css
- gui/dashboard.html
… teardown - Restrict the balance scrape to selectors scoped to the rewards balance component (plus the Bing SERP counter), removing the generic .pointsValue fallbacks that could match — and persist — a daily-points/streak/referral tile as the balance. This matters more now that a 0 balance is accepted. - Ignore out-of-date get_stats() responses on the main window: compare the returned account id against the active account before updating the compact stats card, so a response for a previously selected account can't overwrite fresh data after a switch. - Log (instead of silently swallowing) failures when tearing down secondary windows on exit, while still continuing to close the remaining windows. - Sort the stats package __all__ for consistency.
Owner
appreciate it |
Owner
|
While testing, I ran into an issue with the window positioning. The new window always spawns too far to the right. If the main window is already placed near the right edge of the screen, the new window opens completely off-screen and becomes invisible. 
Your |
safarsin
reviewed
Jul 1, 2026
safarsin
left a comment
safarsin
left a comment
Owner
There was a problem hiding this comment.
I've reviewed the code, and everything looks ok to me
Owner
|
I wanted to discuss these changes with you:
|
Collaborator
Author
|
Hey,
|
The dashboard was always placed just past the main window's right edge, so when the main window sat near the right of the screen the new window opened partly or fully off-screen. Make the docking screen-aware: read the screen work area alongside the main window geometry and place the dashboard to the right if it fits, otherwise to the left, otherwise clamp x/y so the window stays fully within the work area. Falls back to the OS default position when geometry can't be read.
The background WebDriver warmup at launch (which also refreshes the balance)
now blocks the Start button until it completes, so a run can't open a second
driver on the same Edge profile while warmup is still in progress.
Track a driverWarmingUp flag: set while the loader runs and cleared when it
stops. enable_start_button() and refresh_account_ui() both honor it, so the
button stays disabled ("Loading…") throughout warmup and is only enabled once
it finishes (subject to the usual account / first-setup checks).
- Scrape the newly selected account's real balance in the background when
switching accounts, so the card shows an up-to-date total instead of the last
stored one. Runs via the existing refresh path (dedicated lock + account
pinning), so a rapid switch can't open two drivers or write the wrong profile.
- Keep the Start button disabled ("Loading…") while any balance scrape holds a
driver (launch, manual refresh, or switch), not just during the initial
warmup, so a run can't clash with the scrape's driver on the Edge profile.
- Log a single "Points balance updated" line per refresh: the low-level
per-scrape log is dropped so a scrape+update no longer reads as a duplicate.
_install_tray() is called only for its side effects (the window closing handler and the detached tray-icon thread), and the returned icon stays alive via run_detached(). Replace the unused _tray_icon assignment with a plain conditional call to silence the "assigned but never used" warning.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gui/script.js (2)
326-341: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
enable_start_button()unconditionally resets label, ignoring busy/running states.Line 329 always sets the label to
'Start run'before the busy check runs. Every other Start-button controller in this diff (set_stats_loadingline 425,refresh_account_uiline 1066,start_loaderline 1100,stop_loaderline 1139) guards the label against being overwritten while a run is executing ('Running…') or busy ('Loading…'). This function is the one exception: if it's invoked whiledriverWarmingUp/balanceFetchingis true, the label flips back to'Start run'while the button stays correctly disabled (cosmetic mismatch); if it's ever invoked while a run is actually executing, it would overwrite'Running…'and the disabled state wouldn't reflect the busy flags at all, potentially re-enabling Start mid-run.Proposed fix
function enable_start_button() { const btn = document.getElementById('start_btn'); const label = btn.querySelector('.btn-label'); - if (label) label.textContent = 'Start run'; const current = accountsCache.find(a => a.id === currentAccountId); - btn.disabled = !(current && current.first_setup_done) || driverWarmingUp || balanceFetching; + if (label && label.textContent === 'Running…') { + // A run owns the button; don't touch it here. + return; + } + const busy = driverWarmingUp || balanceFetching; + btn.disabled = !(current && current.first_setup_done) || busy; + if (label) label.textContent = busy ? 'Loading…' : 'Start run';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/script.js` around lines 326 - 341, The enable_start_button() controller is resetting the Start button label even when the UI is busy or a run is active. Update enable_start_button() so it only sets the label to “Start run” when neither driverWarmingUp nor balanceFetching is true and no run is in progress, matching the guarding used in set_stats_loading(), refresh_account_ui(), start_loader(), and stop_loader(). Keep the disabled-state logic tied to the same busy/running conditions so the label and enabled state stay consistent.
1127-1145: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
stop_loader()re-enables Start without checkingbalanceFetching.Per the comment at Lines 12-13,
balanceFetchingalso covers the "launch refresh" that happens during driver warmup, and Start must "stay disabled during it too." But Line 1140 only checksdriverWarmingUpreset and account setup — it ignoresbalanceFetching. If the driver-status poll reports warmup finished while the balance-scrape promise (which holds the same driver) hasn't yet resolved, this re-enables Start while a scrape is still in progress, allowing a second driver session on the same Edge profile — exactly what these flags were introduced to prevent.Proposed fix
if (startBtn) { const label = startBtn.querySelector('.btn-label'); const txt = label ? label.textContent : startBtn.textContent; if (txt === 'Start run' || txt === 'Loading…') { - startBtn.disabled = !(current && current.first_setup_done); - if (label) label.textContent = 'Start run'; + startBtn.disabled = !(current && current.first_setup_done) || balanceFetching; + if (label) label.textContent = balanceFetching ? 'Loading…' : 'Start run'; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/script.js` around lines 1127 - 1145, stop_loader() re-enables the Start button too early because it only considers driverWarmingUp and first_setup_done, not balanceFetching. Update the start button enable logic in stop_loader() so it stays disabled whenever balanceFetching is true, even after driver warmup ends, and keep the existing checks tied to currentAccountId/current.first_setup_done and the start_btn label handling.
♻️ Duplicate comments (2)
src/api.py (2)
1813-1820: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse the same profile lease for Start and manual refresh.
_balance_lockonly serializes refreshes with refreshes. Sincemain()only uses_run_lock, Start can still begin after this refresh claims_balance_lock, and both paths can open the same Edge profile.Proposed fix
- if self._run_lock.locked() or self.is_driver_loading: + if self.is_driver_loading: + return {"ok": False, "error": "busy"} + if not self._run_lock.acquire(blocking=False): return {"ok": False, "error": "busy"} @@ if not self._balance_lock.acquire(blocking=False): + self._run_lock.release() return {"ok": False, "error": "busy"} @@ self._set_stats_loading(False) self._balance_lock.release() + self._run_lock.release()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api.py` around lines 1813 - 1820, The manual refresh path in the same-profile guard only takes _balance_lock, so it can still race with Start, which uses _run_lock in main(). Update the refresh flow to claim the same profile lease used by Start—preferably by checking/acquiring _run_lock (or a shared lease helper) before opening the Edge profile—so both start and refresh are mutually exclusive. Keep the busy return behavior consistent in the refresh method where the lock is acquired.
381-387: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCapture the warmup account before opening the driver.
The current “pin” is captured inside
_refresh_balance_on_launch(), aftersetup_driver()may already have opened the old profile. If the user switches accounts during warmup, the scrape can still be written through the newly selected account’sStatsManager.Proposed fix
- warmup_driver = self.driver_manager.setup_driver(headless=True) + warmup_account_id = self.account_manager.current_id() + warmup_stats = self.stats + warmup_meta = self.account_meta + warmup_driver_manager = self.driver_manager + warmup_driver = warmup_driver_manager.setup_driver(headless=True) try: @@ - self._refresh_balance_on_launch(warmup_driver) + self._refresh_balance_on_launch( + warmup_driver, + account_id=warmup_account_id, + stats=warmup_stats, + meta=warmup_meta, + ) @@ - def _refresh_balance_on_launch(self, driver): + def _refresh_balance_on_launch( + self, driver, account_id=None, stats=None, meta=None + ): @@ - stats = self.stats - meta = self.account_meta - account_id = self.account_manager.current_id() + stats = stats if stats is not None else self.stats + meta = meta if meta is not None else self.account_meta + account_id = account_id or self.account_manager.current_id()Also applies to: 1757-1763
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api.py` around lines 381 - 387, Capture the warmup account before calling setup_driver() so the refresh uses the same account context throughout the launch flow. In the launch path and any reused logic around _refresh_balance_on_launch(), read and store the current pin/account identifier first, then pass that stable value into the warmup refresh instead of letting _refresh_balance_on_launch() resolve it after the driver is already opened. Update the code path that writes through StatsManager to use this captured account so a mid-warmup account switch cannot redirect the scraped balance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api.py`:
- Around line 1375-1382: _refresh_balance_async currently refreshes the active
account at execution time and ignores busy failures, so account switching can
update the wrong account or lose the refresh. Update the switch flow in the
account-switch method that starts the thread to pass the selected account_id
into _refresh_balance_async, and have the worker verify that account is still
current before refreshing. Add a short retry loop inside _refresh_balance_async
for busy responses while the same account remains selected, then stop once the
refresh succeeds or the account changes.
---
Outside diff comments:
In `@gui/script.js`:
- Around line 326-341: The enable_start_button() controller is resetting the
Start button label even when the UI is busy or a run is active. Update
enable_start_button() so it only sets the label to “Start run” when neither
driverWarmingUp nor balanceFetching is true and no run is in progress, matching
the guarding used in set_stats_loading(), refresh_account_ui(), start_loader(),
and stop_loader(). Keep the disabled-state logic tied to the same busy/running
conditions so the label and enabled state stay consistent.
- Around line 1127-1145: stop_loader() re-enables the Start button too early
because it only considers driverWarmingUp and first_setup_done, not
balanceFetching. Update the start button enable logic in stop_loader() so it
stays disabled whenever balanceFetching is true, even after driver warmup ends,
and keep the existing checks tied to currentAccountId/current.first_setup_done
and the start_btn label handling.
---
Duplicate comments:
In `@src/api.py`:
- Around line 1813-1820: The manual refresh path in the same-profile guard only
takes _balance_lock, so it can still race with Start, which uses _run_lock in
main(). Update the refresh flow to claim the same profile lease used by
Start—preferably by checking/acquiring _run_lock (or a shared lease helper)
before opening the Edge profile—so both start and refresh are mutually
exclusive. Keep the busy return behavior consistent in the refresh method where
the lock is acquired.
- Around line 381-387: Capture the warmup account before calling setup_driver()
so the refresh uses the same account context throughout the launch flow. In the
launch path and any reused logic around _refresh_balance_on_launch(), read and
store the current pin/account identifier first, then pass that stable value into
the warmup refresh instead of letting _refresh_balance_on_launch() resolve it
after the driver is already opened. Update the code path that writes through
StatsManager to use this captured account so a mid-warmup account switch cannot
redirect the scraped balance.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61df6c38-62ee-46e9-b2dc-d9831d9dc461
📒 Files selected for processing (3)
AutoRewarder.pygui/script.jssrc/api.py
🚧 Files skipped from review as they are similar to previous changes (1)
- AutoRewarder.py
safarsin
approved these changes
Jul 1, 2026
The background refresh started on account switch refreshed whoever was active at execution time and gave up on a transient "busy", so a quick switch could update the wrong account or silently skip the refresh. Pass the selected account_id into _refresh_balance_async and have the worker re-check it is still the current account before each attempt. Retry a bounded number of times only on a "busy" result (driver held by warmup / another refresh) — busy means no scrape happened, so retries add no extra Microsoft hits — stopping as soon as the refresh succeeds or the account changes.
Collaborator
Author
|
Waiting for your opinion on first point, activity area. |
safarsin
reviewed
Jul 1, 2026
Comment on lines
+1383
to
+1400
| def _refresh_balance_async(self, account_id): | ||
| """ | ||
| Background balance refresh for a specific account. Only runs while that | ||
| account is still selected, and retries a few times if the driver is | ||
| momentarily busy (warmup / another refresh). A "busy" result means no | ||
| scrape happened, so retrying doesn't add extra hits to Microsoft. | ||
| """ | ||
| for _ in range(15): | ||
| if self.account_manager.current_id() != account_id: | ||
| return # switched away; that switch triggers its own refresh | ||
| try: | ||
| result = self.refresh_balance() | ||
| except Exception as e: | ||
| self.log(f"[WARNING] Background balance refresh failed: {e}") | ||
| return | ||
| if not (isinstance(result, dict) and result.get("error") == "busy"): | ||
| return # succeeded, or a non-retryable outcome | ||
| time.sleep(2) |
Owner
There was a problem hiding this comment.
I'd like to suggest an optimization here Instead of a fixed 15-try loop with a static delay, we could use exponential backoff. It'll reduce unnecessary wait times
def _refresh_balance_async(self, account_id):
"""Background wrapper around refresh_balance() with retry logic for "busy" errors.
Args:
account_id (str): The ID of the account to refresh.
"""
retries = 3
delay = 0.5
for attempt in range(retries):
if self.account_manager.current_id() != account_id:
return
try:
result = self.refresh_balance()
except Exception as e:
self.log(f"[WARNING] Balance refresh failed for {account_id}: {e}")
return
if result.get("ok"):
return
if result.get("error") != "busy":
return
if attempt < retries - 1:
time.sleep(delay)
delay *= 2
Owner
That's a great idea. For this PR, could you please add Thanks a lot for your hard work on this 🤝 |
Replace the fixed 15-try/2s loop with a short exponential backoff (3 attempts, 0.5s doubling) for transient "busy" driver responses, reducing unnecessary waiting. Still re-checks the account is current before each attempt and stops on success, account change, or any non-busy result.
The body sets user-select: none globally, so the activity log couldn't be selected. Set user-select: text on .activity-feed so users can select and copy log lines.
Collaborator
Author
Done too ! |
safarsin
approved these changes
Jul 2, 2026
Owner
|
Thanks for all your hard work! Merging it now. |
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
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.
Introduce a statistics feature that tracks Microsoft Rewards points and activity per account, surfaced both as a compact card in the main window and a dedicated dashboard window.
Data layer:
Collection points:
UI:
Fixes:
Docs:
Summary by CodeRabbit