fix: improve pihole sessions cleanup#57
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/clients/pihole/pihole.go (1)
75-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDebug log claims success even on a failed logout response.
Line 76 logs "Logged out from Pi-Hole" unconditionally, right before the Warn on Line 78 for
statusCode >= 400. Reading the two logs together is contradictory and undercuts the value of the new debug logging for troubleshooting cleanup failures.🪵 Suggested reorder
p.sid = "" - log.Debug("Logged out from Pi-Hole") if statusCode >= 400 { log.Warn("Pi-Hole logout returned non-success status", "status", statusCode) + } else { + log.Debug("Logged out from Pi-Hole") }🤖 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 `@pkg/clients/pihole/pihole.go` around lines 75 - 79, The logout flow in pihole.go logs “Logged out from Pi-Hole” unconditionally even when Logout returns a failure status, which makes the debug output misleading. In the Logout method, gate the success debug log on a non-error statusCode or move it into the success path so it only prints when the logout actually succeeds; keep the existing warning for statusCode >= 400 and use the Logout and log.Debug/log.Warn calls to place the messages correctly.pkg/clients/clients.go (1)
22-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect, but the shadow at Line 29 is a subtle gotcha—worth a comment.
Without
piholeClient := piholeClient, the deferred closure would reference the named return directly; a laterreturn nil, nil, nil, nil, err(Line 47) zeroes that named variable before the deferred call executes, causing a nil-pointer panic onpiholeClient.Logout(). The shadow correctly avoids this, but it's easy for a future edit to remove it without realizing why it's there.💡 Suggested clarifying comment
- piholeClient := piholeClient + // Capture the client now: subsequent early `return nil, ...` statements + // zero the named return `piholeClient`, which would make this deferred + // closure panic on a nil pointer if it referenced the named return directly. + piholeClient := piholeClient🤖 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 `@pkg/clients/clients.go` around lines 22 - 37, The piholeClient shadow inside the Login/defer block is intentional but easy to remove accidentally, so add a clarifying comment near the piholeClient := piholeClient assignment in clients.go. Make it explicit that the local copy is needed because the deferred Logout closure must not reference the named return variable, which gets zeroed on the early return path before defer runs.
🤖 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.
Nitpick comments:
In `@pkg/clients/clients.go`:
- Around line 22-37: The piholeClient shadow inside the Login/defer block is
intentional but easy to remove accidentally, so add a clarifying comment near
the piholeClient := piholeClient assignment in clients.go. Make it explicit that
the local copy is needed because the deferred Logout closure must not reference
the named return variable, which gets zeroed on the early return path before
defer runs.
In `@pkg/clients/pihole/pihole.go`:
- Around line 75-79: The logout flow in pihole.go logs “Logged out from Pi-Hole”
unconditionally even when Logout returns a failure status, which makes the debug
output misleading. In the Logout method, gate the success debug log on a
non-error statusCode or move it into the success path so it only prints when the
logout actually succeeds; keep the existing warning for statusCode >= 400 and
use the Logout and log.Debug/log.Warn calls to place the messages correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8cc14bca-19ab-44d7-b8fe-9561851199e8
📒 Files selected for processing (3)
pkg/clients/clients.gopkg/clients/common/common.gopkg/clients/pihole/pihole.go
5309e1a to
8913260
Compare
1 participant
Summary by CodeRabbit