Skip to content

[script.punchplay] 1.0.2#2818

Open
PunchPlay wants to merge 1 commit intoxbmc:omegafrom
PunchPlay:omega
Open

[script.punchplay] 1.0.2#2818
PunchPlay wants to merge 1 commit intoxbmc:omegafrom
PunchPlay:omega

Conversation

@PunchPlay
Copy link
Copy Markdown

Changes

  • Add QR code to login dialog — scan with phone to open punchplay.tv/link with the code pre-filled
  • Fix login poll silently swallowing HTTP errors (expired/access_denied were never detected)
  • Fix stale QR image when re-triggering login (Kodi texture cache)
  • Shorten addon store description
  • Proper error logging throughout the device-code poll loop

@kodiai
Copy link
Copy Markdown

kodiai Bot commented Apr 21, 2026

Kodiai Addon Check

✅ No issues found by kodi-addon-checker.

@kodiai
Copy link
Copy Markdown

kodiai Bot commented Apr 21, 2026

Kodiai Review Summary

What Changed

Adds QR code login support, fixes HTTP error handling in device-code poll loop, and improves error logging throughout the authentication flow.

Reviewed: core logic, docs

Strengths

  • ✅ Device-code poll loop now properly reads HTTP error bodies to detect terminal states (expired, access_denied) at api.py:409-433, fixing silent error swallowing
  • ✅ Heartbeat thread stops cleanly on any unhandled error at player.py:174-176, preventing silent spinning on bad player state
  • ✅ QR cleanup uses timestamp-based filenames at api.py:267 to avoid Kodi texture cache staleness

Observations

Impact

[MAJOR] script.punchplay/api.py (271): Missing exception handler for directory listing
os.listdir(self._data_dir) can raise OSError/FileNotFoundError if the directory doesn't exist or isn't accessible, crashing the login flow. The function returns None for other error conditions, so this should be handled gracefully.

[MAJOR] script.punchplay/service.py (84): Queue flush retry delay regression
_last_flush timestamp is now updated even when flush_queue() fails. This changes retry behavior from 10 seconds (old: only update on success) to 5 minutes (new: always update), delaying queued scrobbles on transient network errors.

Suggestions

  • Optional: Consider adding integration tests for the QR dialog display path to catch UI layout regressions

Verdict

🔴 Address before merging -- 2 blocking issue(s) found

Review Details
  • Files reviewed: 11

  • Findings: 0 critical, 4 major, 0 medium, 0 minor (includes 2 from summary observations)

  • Lines changed: +342 -63

  • Profile: balanced (auto, lines changed: 405)

  • Contributor experience: coarse-fallback (using coarse fallback signals only)

  • Review completed: 2026-04-21T00:12:22.829Z

  • Total wall-clock: 3m 21s

  • Phase timings:

    • queue wait: 0ms
    • workspace preparation: 2.0s
    • retrieval/context assembly: 2.5s
    • executor handoff: 2.4s
    • remote runtime: 3m 11s
    • publication: 857ms
  • Tokens: 54 in / 6,960 out | 0.2571

  • Keyword parsing: No keywords detected

Comment thread script.punchplay/api.py Outdated
Comment thread script.punchplay/service.py Outdated
@PunchPlay
Copy link
Copy Markdown
Author

@kodiai please review

Comment thread script.punchplay/login_dialog.py Outdated
@kodiai
Copy link
Copy Markdown

kodiai Bot commented Apr 21, 2026

Kodiai Review Summary

What Changed

Version 1.0.2 adds QR code support to the login dialog and fixes login poll error handling to properly detect expired/access_denied states.

Reviewed: core logic, docs

Strengths

  • ✅ Error handling in the device-code poll loop now properly reads HTTP response bodies to distinguish between "authorization_pending" (continue polling) and terminal errors like "expired" or "access_denied" (api.py:425-449)
  • ✅ QR image cache management uses timestamps in filenames to avoid Kodi's texture cache serving stale codes from previous login attempts (api.py:268)
  • ✅ Heartbeat thread now stops immediately on any unhandled error rather than continuing to spin, preventing silent failures (player.py:172-176)

Observations

Impact

[MINOR] script.punchplay/login_dialog.py (16-17): Incorrect action ID comments
Comments claim action 9 is ACTION_HIGHLIGHT_ITEM and action 10 is ACTION_PARENT_DIR, but according to Kodi's ActionIDs.h, 9 is ACTION_PARENT_DIR and 10 is ACTION_PREVIOUS_MENU. The numeric values are correct (dialog closes properly), but the comments are misleading for future maintainers.

Verdict

🟡 Ready to merge with minor items -- Optional cleanup suggestions below

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