refactor: use tailscale api for user checking instead of tsnet#978
Merged
Conversation
dosubot
Bot
added
the
size:XL
Contributor
📝 WalkthroughWalkthroughThis PR replaces the embedded Tailscale server integration with a REST API client, updates Tailscale configuration and context fields, removes Tailscale listener/AppURL wiring, refactors OAuth request helpers to use context, and removes Tailscale build-tag and dependency references. ChangesTailscale REST migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/service/simple_req.go (2)
10-30: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider context-based cancellation for this shared HTTP helper.
simpleReqnow backs both OAuth calls and the new Tailscale REST polling (getDeviceList/getUsersList). It builds anhttp.Requestwithout acontext.Context, so cancellation/deadlines rely entirely on the caller's*http.Clienttimeout. If any caller passes a client without a configured timeout, a hung request could block a polling cycle indefinitely.♻️ Optional refactor: accept a context
-func simpleReq[T any](client *http.Client, url string, headers map[string]string) (*T, error) { +func simpleReq[T any](ctx context.Context, client *http.Client, url string, headers map[string]string) (*T, error) { var decodedRes T - req, err := http.NewRequest("GET", url, nil) + req, err := http.NewRequestWithContext(ctx, "GET", url, nil) if err != nil { return nil, err }This widens the function signature across all call sites, so weigh it against the current reliance on client-level timeouts.
🤖 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 `@internal/service/simple_req.go` around lines 10 - 30, simpleReq currently creates requests without a context, so long-running OAuth and Tailscale polling calls can hang if the passed http.Client has no timeout. Update simpleReq to accept a context and pass it into http.NewRequestWithContext, then thread that context through the callers like getDeviceList and getUsersList so cancellation and deadlines are enforced consistently. Keep the existing header handling, response closing, and status checks intact while updating the signature and all call sites.
28-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude response body in the non-2xx error for easier debugging.
Currently only the status text is surfaced; the response body (often containing API error details) is discarded.
♻️ Optional improvement
if res.StatusCode < 200 || res.StatusCode >= 300 { - return nil, fmt.Errorf("request failed with status: %s", res.Status) + body, _ := io.ReadAll(res.Body) + return nil, fmt.Errorf("request failed with status: %s, body: %s", res.Status, string(body)) }🤖 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 `@internal/service/simple_req.go` around lines 28 - 30, The non-2xx branch in the simple request helper only returns the HTTP status text and drops the response body, which hides useful API error details. Update the error handling around the res.StatusCode check to read and include the response body in the returned error message, and make sure the response body is still safely handled/closed after being read.internal/service/tailscale_service.go (1)
189-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse explicit local copies before storing range item pointers.
This avoids ambiguity around range-variable pointer semantics and matches this repo’s preferred pattern. Based on learnings, when storing a pointer to a Go range value, prefer an explicit local copy pattern.
Proposed refactor
+deviceLoop: for _, d := range devices.Devices { if len(d.Tags) != 0 { continue } for _, a := range d.Addresses { if a == addr { - device = &d - break + result := d + device = &result + break deviceLoop } } }for _, u := range users.Users { if u.LoginName == device.User { - user = &u + result := u + user = &result break } }Also applies to: 215-217
🤖 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 `@internal/service/tailscale_service.go` around lines 189 - 195, The loops in tailscale_service.go are storing pointers to Go range items directly, which should be replaced with an explicit local copy before assignment. In the code that iterates over devices.Devices and the related block around the other referenced range loop, create a new local variable from the current range item and assign the pointer to that copy instead of taking the range variable’s address. Keep the logic in the existing device-selection code paths unchanged, but update the pointer capture pattern in the affected loops to match the repo’s preferred approach.Source: Learnings
🤖 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 @.env.example:
- Around line 226-233: The Tailscale env block in the example env file is out of
dotenv-linter order. Reorder the TINYAUTH_TAILSCALE_* entries so the API token
and cache duration keys come before TINYAUTH_TAILSCALE_ENABLED, while keeping
the rest of the Tailscale settings grouped together and preserving the existing
comments.
In `@internal/service/tailscale_service.go`:
- Line 117: The Tailscale service client is created without any timeout, so
requests can hang indefinitely during startup or cache misses. Update the client
initialization in the Tailscale service setup where s.client is assigned in the
Tailscale service constructor/initializer to use an http.Client with a
reasonable timeout value instead of the zero-value client. Keep the change
localized to the Tailscale client creation path so all API calls inherit the
timeout behavior.
---
Nitpick comments:
In `@internal/service/simple_req.go`:
- Around line 10-30: simpleReq currently creates requests without a context, so
long-running OAuth and Tailscale polling calls can hang if the passed
http.Client has no timeout. Update simpleReq to accept a context and pass it
into http.NewRequestWithContext, then thread that context through the callers
like getDeviceList and getUsersList so cancellation and deadlines are enforced
consistently. Keep the existing header handling, response closing, and status
checks intact while updating the signature and all call sites.
- Around line 28-30: The non-2xx branch in the simple request helper only
returns the HTTP status text and drops the response body, which hides useful API
error details. Update the error handling around the res.StatusCode check to read
and include the response body in the returned error message, and make sure the
response body is still safely handled/closed after being read.
In `@internal/service/tailscale_service.go`:
- Around line 189-195: The loops in tailscale_service.go are storing pointers to
Go range items directly, which should be replaced with an explicit local copy
before assignment. In the code that iterates over devices.Devices and the
related block around the other referenced range loop, create a new local
variable from the current range item and assign the pointer to that copy instead
of taking the range variable’s address. Keep the logic in the existing
device-selection code paths unchanged, but update the pointer capture pattern in
the affected loops to match the repo’s preferred approach.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c0e1dd43-9ad0-40b0-aa0d-b8f9e6bf2f20
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
.env.example.github/workflows/nightly.yml.github/workflows/release.ymlMakefilego.modinternal/bootstrap/app_bootstrap.gointernal/bootstrap/router_bootstrap.gointernal/middleware/context_middleware.gointernal/model/config.gointernal/model/context.gointernal/service/oauth_extractors.gointernal/service/simple_req.gointernal/service/tailscale_service.go
💤 Files with no reviewable changes (2)
- internal/bootstrap/app_bootstrap.go
- internal/service/oauth_extractors.go
Rycochet
reviewed
Jul 8, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/service/tailscale_service.go (1)
200-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer an explicit local copy over taking the address of the range variable.
device = &d(Line 206) anduser = &u(Line 228) capture the address of the loop variable. On Go 1.25 this is safe, but the project convention is to bind an explicit copy for clarity and to avoid reviewer confusion about loop-variable capture semantics.Additionally, the inner
breakon Line 207 only exits the innerAddressesloop; the outer device loop keeps scanning and could overwritedeviceon a later address match. Consider breaking out of the outer loop once a device is found.♻️ Proposed refactor
for _, d := range devices.Devices { if len(d.Tags) != 0 { continue } for _, a := range d.Addresses { if a == addr { - device = &d + match := d + device = &match break } } + if device != nil { + break + } }for _, u := range users.Users { if u.LoginName == device.User { - user = &u + match := u + user = &match break } }Based on learnings, prefer the explicit local copy pattern (e.g.,
result := config; return &result) rather than&<range-variable>directly, even when newer Go versions make per-iteration address taking safe.🤖 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 `@internal/service/tailscale_service.go` around lines 200 - 231, The Tailscale device/user lookup in tailscaleService should avoid taking the address of range variables and should stop scanning once a match is found. In the device search loop, bind an explicit local copy before assigning to device instead of using &d, and make sure the outer device loop exits after the first matching address rather than only breaking the inner Addresses loop. Apply the same explicit-copy pattern when setting user from users.Users instead of using &u, so the code is clear and consistent with the project convention.Source: Learnings
🤖 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 `@internal/service/tailscale_service.go`:
- Around line 200-231: The Tailscale device/user lookup in tailscaleService
should avoid taking the address of range variables and should stop scanning once
a match is found. In the device search loop, bind an explicit local copy before
assigning to device instead of using &d, and make sure the outer device loop
exits after the first matching address rather than only breaking the inner
Addresses loop. Apply the same explicit-copy pattern when setting user from
users.Users instead of using &u, so the code is clear and consistent with the
project convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e9dec8a-8bc7-47af-925e-fa7586146cfc
📒 Files selected for processing (3)
.env.exampleinternal/model/config.gointernal/service/tailscale_service.go
Rycochet
approved these changes
Jul 8, 2026
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
2 participants
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.
Summary by CodeRabbit