Conversation
📝 WalkthroughWalkthroughAdds Beeper Desktop login integration: discovers Desktop data dir, reads ChangesDesktop credential import & login flow
Sequence Diagram(s)sequenceDiagram
participant CLI as bbctl CLI
participant FS as File system
participant SQL as SQLite (account.db)
participant API as beeperapi (Whoami)
participant CFG as bbctl config
CLI->>FS: resolve Desktop data dir (--desktop-data-dir / profile / OS default)
CLI->>SQL: open account.db (read-only) and query first account row
SQL-->>CLI: DesktopAccount (user_id, access_token, homeserver)
CLI->>API: Whoami(access_token)
API-->>CLI: return identity / cluster info
CLI->>CFG: update env (ClusterID, Username, AccessToken, BridgeDataDir, DesktopDataDir)
CFG-->>CLI: save config
CLI-->>CLI: print "Using Beeper Desktop login …"
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 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 `@cmd/bbctl/desktopauth.go`:
- Line 203: The printed message prepends an extra "@" because account.UserID
already contains the leading '@'; update the fmt.Printf call that prints
"Imported Desktop login..." to remove the literal "@" before %s (use %s with
account.UserID) so it doesn't output "@@...". Locate the printf in the desktop
auth import logic (the fmt.Printf line referencing account.UserID, env,
homeserver) and adjust the format string accordingly.
- Around line 110-133: readDesktopAccount currently uses sql.Open and defers
db.Close without checking its error and uses QueryRow (no context); change
readDesktopAccount to accept ctx context.Context (update callers loginDesktop
and maybeUseDesktopLogin to pass ctx), open the DB as before but replace
db.QueryRow with db.QueryRowContext(ctx, ...) and propagate context-aware
errors, and ensure the deferred db.Close() error is checked by capturing its
return (e.g., assign closeErr := db.Close() in the defer and, if non-nil,
wrap/return it using fmt.Errorf to add context like "closing desktop account
database: %w"); also enrich existing fmt.Errorf calls to include context where
appropriate.
🪄 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: CHILL
Plan: Pro
Run ID: 94cbf344-af34-45d4-8b1f-f02e1c2a0a4f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
cmd/bbctl/authconfig.gocmd/bbctl/desktopauth.gocmd/bbctl/login-email.gocmd/bbctl/main.gogo.mod
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: build-docker (amd64)
- GitHub Check: build
- GitHub Check: lint
🧰 Additional context used
🪛 golangci-lint (2.11.4)
cmd/bbctl/desktopauth.go
[error] 120-120: Error return value of db.Close is not checked
(errcheck)
[error] 123-123: (*database/sql.DB).QueryRow must not be called. use (*database/sql.DB).QueryRowContext
(noctx)
🔇 Additional comments (10)
cmd/bbctl/desktopauth.go (4)
21-46: LGTM!The command definition and flags are well-structured, supporting multiple ways to specify the Desktop data location (explicit DB path, data directory, or auto-detected via profile).
65-97: LGTM!The OS-specific path resolution correctly follows platform conventions:
~/Library/Application Support/on macOS,APPDATAon Windows, andXDG_CONFIG_HOME(defaulting to~/.config) on Linux.
135-144: LGTM!The domain extraction correctly parses the homeserver URL and strips the
matrix.prefix to obtain the base domain for environment matching.
155-185: LGTM!The function properly verifies credentials via
Whoamibefore persisting them, ensuring invalid tokens don't get saved. The environment detection and fallback logic is sound.cmd/bbctl/authconfig.go (1)
34-36: LGTM!The relaxed credential check aligns with the PR objective to support newer access tokens that don't use the
syt_prefix. Downstream API calls (viabeeperapi.Whoami) provide runtime validation of token validity.cmd/bbctl/login-email.go (3)
27-37: LGTM!The
--no-desktopflag and dynamic flag composition viainit()cleanly integrate the desktop login options into the existing login command.
39-73: LGTM!The error handling correctly distinguishes between explicit flag usage (where errors should propagate) and auto-detection (where silent fallback to email login is appropriate). The user confirmation prompt is good UX.
75-92: LGTM!The login flow correctly prioritizes desktop login when available, with appropriate early exit on success or propagation of errors for explicit flag usage.
cmd/bbctl/main.go (1)
141-153: LGTM!The
loginDesktopCommandis properly registered alongside existing authentication commands.go.mod (1)
10-10: go-sqlite3 version v1.14.44 is valid and has no known vulnerabilities.Version v1.14.44 exists and is well-maintained. The CGO requirement (C compiler needed) is a known characteristic of this SQLite driver but is necessary for the database access pattern used in desktopauth.go.
batuhan
force-pushed
the
codex/desktop-login-import
branch
from
12a46b5 to
6efe4e2
Compare
batuhan
force-pushed
the
codex/desktop-login-import
branch
from
6efe4e2 to
3aacb0d
Compare
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 `@cmd/bbctl/authconfig.go`:
- Around line 34-35: EnvConfig.HasCredentials() currently only returns true for
AccessToken prefixes "syt_" and "bat_", which ignores valid imported Desktop
tokens and causes RequiresAuth() and main.go startup logic to treat users as
logged out; update HasCredentials() to consider any non-empty AccessToken as
valid (e.g., check ec.AccessToken != "" or strings.TrimSpace(ec.AccessToken) !=
"") while keeping existing prefix checks if you want to special-case types, and
ensure callers like RequiresAuth() and the startup flow in main.go will
recognize imported Desktop tokens by relying on the updated HasCredentials()
behavior.
🪄 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: CHILL
Plan: Pro
Run ID: 4ba7397f-242f-4297-af72-e3cacd28f7ae
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
cmd/bbctl/authconfig.gocmd/bbctl/desktopauth.gocmd/bbctl/login-email.gocmd/bbctl/main.gogo.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/bbctl/main.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: deploy-docker
- GitHub Check: deploy-docker
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: build
- GitHub Check: lint
🧰 Additional context used
🪛 golangci-lint (2.11.4)
cmd/bbctl/desktopauth.go
[error] 122-122: Error return value of db.Close is not checked
(errcheck)
🔇 Additional comments (2)
cmd/bbctl/desktopauth.go (2)
118-123: Check the deferreddb.Close()error.The close failure is still dropped here, so read-only DB issues can be lost silently. This was already flagged by lint, but it remains unresolved in the new code path.
205-205: Remove the extra@from the import message.
account.UserIDalready includes the prefix, so this prints@@...for every successful import.
| func (ec *EnvConfig) HasCredentials() bool { | ||
| return strings.HasPrefix(ec.AccessToken, "syt_") | ||
| return strings.HasPrefix(ec.AccessToken, "syt_") || strings.HasPrefix(ec.AccessToken, "bat_") |
There was a problem hiding this comment.
Broaden HasCredentials() to accept imported Desktop tokens.
RequiresAuth() and the startup path in main.go both rely on this gate. Limiting it to syt_/bat_ still treats other non-empty access tokens as logged out, so a successful Desktop import can be ignored by later commands.
Suggested fix
func (ec *EnvConfig) HasCredentials() bool {
- return strings.HasPrefix(ec.AccessToken, "syt_") || strings.HasPrefix(ec.AccessToken, "bat_")
+ return ec.AccessToken != ""
}📝 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.
| func (ec *EnvConfig) HasCredentials() bool { | |
| return strings.HasPrefix(ec.AccessToken, "syt_") | |
| return strings.HasPrefix(ec.AccessToken, "syt_") || strings.HasPrefix(ec.AccessToken, "bat_") | |
| func (ec *EnvConfig) HasCredentials() bool { | |
| return ec.AccessToken != "" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/bbctl/authconfig.go` around lines 34 - 35, EnvConfig.HasCredentials()
currently only returns true for AccessToken prefixes "syt_" and "bat_", which
ignores valid imported Desktop tokens and causes RequiresAuth() and main.go
startup logic to treat users as logged out; update HasCredentials() to consider
any non-empty AccessToken as valid (e.g., check ec.AccessToken != "" or
strings.TrimSpace(ec.AccessToken) != "") while keeping existing prefix checks if
you want to special-case types, and ensure callers like RequiresAuth() and the
startup flow in main.go will recognize imported Desktop tokens by relying on the
updated HasCredentials() behavior.
There was a problem hiding this comment.
desktop token is also a matrix token
There was a problem hiding this comment.
@batuhan, thanks for the clarification! Since the Desktop access token is a standard Matrix token, it will already carry a syt_ or bat_ prefix, so the current prefix-based check in HasCredentials() is correct and my suggestion was unnecessary. I'll withdraw it.
✏️ Learnings added
Learnt from: batuhan
Repo: beeper/bridge-manager PR: 75
File: cmd/bbctl/authconfig.go:35-36
Timestamp: 2026-05-03T18:23:09.796Z
Learning: In the beeper/bridge-manager repository (`cmd/bbctl/authconfig.go`), Beeper Desktop access tokens are standard Matrix tokens and will carry the same `syt_` or `bat_` prefixes. The prefix-based check in `EnvConfig.HasCredentials()` is intentional and sufficient to cover Desktop login tokens as well.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/bbctl/desktopauth.go (1)
122-122:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle the
db.Close()error.The deferred close still drops the return value, so the existing lint finding remains and any shutdown-time I/O failure is lost.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/bbctl/desktopauth.go` at line 122, The defer currently drops db.Close()'s error (defer db.Close()), losing shutdown I/O failures; replace it with a deferred closure that checks and surfaces or logs the error from db.Close() (e.g. defer func(){ if cerr := db.Close(); cerr != nil { /* log or wrap into returned error */ } }()), referencing the same db variable and db.Close() call so shutdown errors are not ignored. Ensure you either append the close error to the function's returned error (if it has a named return) or log it via the existing logger used in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/bbctl/desktopauth.go`:
- Line 122: The defer currently drops db.Close()'s error (defer db.Close()),
losing shutdown I/O failures; replace it with a deferred closure that checks and
surfaces or logs the error from db.Close() (e.g. defer func(){ if cerr :=
db.Close(); cerr != nil { /* log or wrap into returned error */ } }()),
referencing the same db variable and db.Close() call so shutdown errors are not
ignored. Ensure you either append the close error to the function's returned
error (if it has a named return) or log it via the existing logger used in this
file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30fc543f-be7b-409a-b44f-311af665a07f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
cmd/bbctl/authconfig.gocmd/bbctl/desktopauth.gocmd/bbctl/login-email.gocmd/bbctl/main.gogo.mod
✅ Files skipped from review due to trivial changes (1)
- cmd/bbctl/authconfig.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- cmd/bbctl/login-email.go
📜 Review details
🧰 Additional context used
🪛 golangci-lint (2.11.4)
cmd/bbctl/desktopauth.go
[error] 122-122: Error return value of db.Close is not checked
(errcheck)
🔇 Additional comments (1)
cmd/bbctl/main.go (1)
143-143: Looks good.Registering
loginDesktopCommandhere wires the new CLI entrypoint into the app cleanly.
Add support for using Beeper Desktop credentials for bbctl without persisting desktop access tokens. Introduce DesktopDataDir on EnvConfig and UsesDesktopLogin() to mark desktop-based envs; make AccessToken optional in config. When saving config, copy environments and omit AccessToken for desktop-logins. Add getDesktopDataDir helper, improve resolution of the desktop data directory, and enhance readDesktopAccount with better error handling for DB close and clearer local variables. Rename and refactor saveDesktopLogin to configureDesktopLogin and add loadDesktopLogin to populate runtime credentials from DesktopDataDir (loaded automatically in prepareApp for non-login commands). Update CLI flag usage text and messages, change maybeUseDesktopLogin to call the refactored configure flow, and clear DesktopDataDir on normal Matrix/email logins. Add isLoginCommand helper to avoid loading desktop creds during login flows.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/bbctl/authconfig.go (1)
35-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
HasCredentials()still rejects valid non-prefixed access tokens.Line 36 still gates on
syt_/bat_prefixes, which can treat valid Desktop-loaded Matrix tokens as logged out.Suggested fix
func (ec *EnvConfig) HasCredentials() bool { - return strings.HasPrefix(ec.AccessToken, "syt_") || strings.HasPrefix(ec.AccessToken, "bat_") + return strings.TrimSpace(ec.AccessToken) != "" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/bbctl/authconfig.go` around lines 35 - 37, The HasCredentials method in EnvConfig incorrectly only accepts tokens that start with "syt_" or "bat_", rejecting valid tokens without those prefixes (e.g., Desktop-loaded Matrix tokens); update EnvConfig.HasCredentials to simply treat any non-empty AccessToken as valid (e.g., return true when strings.TrimSpace(ec.AccessToken) != ""), while preserving existing behavior for prefixed tokens implicitly; modify the function body in HasCredentials to remove the prefix-only checks and use a non-empty trimmed-access-token check instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/bbctl/main.go`:
- Around line 84-89: The current code hard-fails when loadDesktopLogin(ctx,
envConfig) returns an error, which prevents recovery commands like logout from
running; change the logic so that if loadDesktopLogin fails you only return the
error for non-recovery/non-login commands. Concretely, update the block around
envConfig.UsesDesktopLogin(), isLoginCommand(ctx) and loadDesktopLogin(ctx,
envConfig) to: attempt loadDesktopLogin, and on error check a recovery predicate
(e.g., extend or add to isLoginCommand to include "logout" or introduce
isRecoveryCommand that recognizes logout); if the command is a recovery/login
command, log or ignore the desktop-load error and continue, otherwise return the
error as before. Ensure references to envConfig.UsesDesktopLogin(),
isLoginCommand(ctx) (or new isRecoveryCommand), and loadDesktopLogin(ctx,
envConfig) are used to locate and implement this change.
---
Duplicate comments:
In `@cmd/bbctl/authconfig.go`:
- Around line 35-37: The HasCredentials method in EnvConfig incorrectly only
accepts tokens that start with "syt_" or "bat_", rejecting valid tokens without
those prefixes (e.g., Desktop-loaded Matrix tokens); update
EnvConfig.HasCredentials to simply treat any non-empty AccessToken as valid
(e.g., return true when strings.TrimSpace(ec.AccessToken) != ""), while
preserving existing behavior for prefixed tokens implicitly; modify the function
body in HasCredentials to remove the prefix-only checks and use a non-empty
trimmed-access-token check instead.
🪄 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: CHILL
Plan: Pro
Run ID: de84303e-6123-45ef-82fb-1c08ac1f9c2c
📒 Files selected for processing (4)
cmd/bbctl/authconfig.gocmd/bbctl/desktopauth.gocmd/bbctl/login-email.gocmd/bbctl/main.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/bbctl/login-email.go
- cmd/bbctl/desktopauth.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (1)
cmd/bbctl/authconfig.go (1)
165-179: Good token-sanitization approach inSave().Copying
cfgand clearingAccessTokenonly for desktop-backed envs before JSON encode is a solid way to avoid persisting Desktop credentials.
Simplify and clean up Beeper Desktop login integration: remove DeviceID usage and stop clearing desktop access tokens when saving config. Unify desktop data directory handling by changing getDesktopDataDir to return a single dataDir, and update callers to pass dataDir through to readDesktopAccount (which now reads account.db from the data dir) and configureDesktopLogin. Update SQL query to omit device_id. Extract whoami verification into applyDesktopWhoami and call it only when cluster/username metadata is missing; configureDesktopLogin now sets BridgeDataDir and DesktopDataDir and saves the config. Improve error messages when resolving the desktop data dir. Also: append desktop flags to the login command inline, and make isLoginCommand detect login commands via Command.HasName().
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/bbctl/authconfig.go (1)
35-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAccept any non-empty configured token in
HasCredentials().Line 36 still restricts credentials to
syt_/bat_prefixes. That will misclassify valid non-empty Desktop/new-format tokens as logged out in downstream auth gates.Suggested fix
func (ec *EnvConfig) HasCredentials() bool { - return strings.HasPrefix(ec.AccessToken, "syt_") || strings.HasPrefix(ec.AccessToken, "bat_") + return strings.TrimSpace(ec.AccessToken) != "" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/bbctl/authconfig.go` around lines 35 - 37, The HasCredentials() method on EnvConfig currently only accepts tokens with "syt_" or "bat_" prefixes; change it to treat any non-empty configured AccessToken as valid (e.g., return whether strings.TrimSpace(ec.AccessToken) is not empty) so Desktop/new-format tokens are recognized; update EnvConfig.HasCredentials to check for a non-empty/ec.AccessToken instead of prefix checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/bbctl/authconfig.go`:
- Around line 35-37: The HasCredentials() method on EnvConfig currently only
accepts tokens with "syt_" or "bat_" prefixes; change it to treat any non-empty
configured AccessToken as valid (e.g., return whether
strings.TrimSpace(ec.AccessToken) is not empty) so Desktop/new-format tokens are
recognized; update EnvConfig.HasCredentials to check for a
non-empty/ec.AccessToken instead of prefix checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 752910bf-9c84-4460-a70f-305f120b1587
📒 Files selected for processing (3)
cmd/bbctl/authconfig.gocmd/bbctl/desktopauth.gocmd/bbctl/login-email.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-docker (amd64)
- GitHub Check: lint
- GitHub Check: build-docker (arm64)
- GitHub Check: build
🔇 Additional comments (4)
cmd/bbctl/authconfig.go (1)
39-41:UsesDesktopLogin()helper is clean and readable.The
DesktopDataDir != ""gate is a clear signal for desktop-login mode and keeps call sites simple.cmd/bbctl/desktopauth.go (1)
105-136: DB read path is hardened well.
readDesktopAccountnow uses context-aware querying and checks DB close errors, which improves cancellation behavior and error hygiene.cmd/bbctl/login-email.go (2)
43-77: Desktop-first fallback flow is well-structured.The opt-out flag, detection flow, confirmation prompt, and early-return path are integrated cleanly without disrupting the email login fallback.
146-146: Clearing desktop linkage after Matrix login is the right reset.Line 146 prevents stale desktop-login mode from persisting after an explicit email-based login.
Check the environment config and, when UsesDesktopLogin is true, avoid calling the Matrix Logout API so the Beeper Desktop session remains active. The environment is still removed from the CLI config and saved (with an inline error check). Print a specific message indicating the desktop session was not affected; otherwise keep the original success message.
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 `@cmd/bbctl/desktopauth.go`:
- Around line 136-140: The code uses parsed.Host which can include a port and
leads to incorrect envForHomeserverDomain matching; update the logic that
derives the homeserver domain (the block working with parsed, err :=
url.Parse(account.Homeserver)) to call parsed.Hostname() instead of parsed.Host,
explicitly reject empty parsed.Hostname() with an error (e.g., "empty host in
homeserver URL"), and keep the existing TrimPrefix call but apply it to
parsed.Hostname() so ports are excluded when returning the domain used for
environment selection.
🪄 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: CHILL
Plan: Pro
Run ID: 06e7ee5f-dd1d-48c1-9619-6fd0f3673e9e
📒 Files selected for processing (3)
cmd/bbctl/authconfig.gocmd/bbctl/desktopauth.gocmd/bbctl/logout.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/bbctl/authconfig.go
📜 Review details
🔇 Additional comments (1)
cmd/bbctl/logout.go (1)
25-40: Desktop-session-preserving logout flow looks correct.Nice change: this cleanly skips Matrix logout for Desktop-backed auth while still removing the bbctl env and saving config.
| parsed, err := url.Parse(account.Homeserver) | ||
| if err != nil { | ||
| return "", fmt.Errorf("desktop account has invalid homeserver URL %q: %w", account.Homeserver, err) | ||
| } | ||
| return strings.TrimPrefix(parsed.Host, "matrix."), nil |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go's net/url package, does URL.Host include the port while URL.Hostname() strips it? Please provide examples.
💡 Result:
Yes. In Go’s net/url package, URL.Host includes the port when it is present (it is documented as “host” or “host:port”). The method URL.Hostname strips any valid port from URL.Host. Examples (using Go’s behavior): 1) Hostname strips an explicit port - Parse: https://example.com:8443/path - u.Host == "example.com:8443" - u.Hostname == "example.com" - u.Port == "8443" 2) Hostname on an IPv6 literal with a port - Parse: http://[::1]:8080/ - u.Host == "[::1]:8080" - u.Hostname == "::1" (brackets removed) - u.Port == "8080" 3) When there is no port, Hostname returns the whole host - Parse: https://example.com/path - u.Host == "example.com" - u.Hostname == "example.com" - u.Port == "" (empty because there’s no numeric port in Host) Authoritative notes from the net/url docs: - The URL.Host field “contains the host and port subcomponents” and “when the port is present, it is separated … with a colon” (and IPv6 is bracketed as “[ip]:port”). - URL.Hostname “returns u.Host, stripping any valid port number if present.”
Citations:
- 1: https://pkg.go.dev/net/url@go1.25.6
- 2: https://pkg.go.dev/net/url
- 3: https://go.dev/src/net/url/url.go
🏁 Script executed:
# Find and examine the desktopauth.go file
fd -t f 'desktopauth.go'Repository: beeper/bridge-manager
Length of output: 90
🏁 Script executed:
# Look at the specific function and surrounding context
cat -n cmd/bbctl/desktopauth.go | sed -n '130,150p'Repository: beeper/bridge-manager
Length of output: 721
🏁 Script executed:
# Search for envForHomeserverDomain to understand how this value is used
rg -n 'envForHomeserverDomain' --type goRepository: beeper/bridge-manager
Length of output: 254
🏁 Script executed:
# Search for desktopAccountHomeserverDomain usage
rg -n 'desktopAccountHomeserverDomain' --type goRepository: beeper/bridge-manager
Length of output: 351
🏁 Script executed:
# Look at the envs map to understand what domains are expected
rg -n 'var envs|const envs' --type go -A 10 cmd/bbctl/desktopauth.goRepository: beeper/bridge-manager
Length of output: 47
🏁 Script executed:
# Look at the full context around line 153-158 where desktopAccountHomeserverDomain is called
cat -n cmd/bbctl/desktopauth.go | sed -n '150,165p'Repository: beeper/bridge-manager
Length of output: 669
🏁 Script executed:
# Look at the desktopAuth function to understand the full flow
cat -n cmd/bbctl/desktopauth.go | sed -n '150,210p'Repository: beeper/bridge-manager
Length of output: 2370
Use Hostname() instead of Host to exclude port from homeserver domain matching.
parsed.Host includes the port number (e.g., matrix.org:8448), which causes envForHomeserverDomain matching to fail. This results in the wrong environment being selected and credentials persisting to the incorrect BridgeDataDir location. Additionally, reject URLs with empty host explicitly for robustness.
Proposed fix
func desktopAccountHomeserverDomain(account *DesktopAccount) (string, error) {
if account.Homeserver == "" {
return "", nil
}
parsed, err := url.Parse(account.Homeserver)
if err != nil {
return "", fmt.Errorf("desktop account has invalid homeserver URL %q: %w", account.Homeserver, err)
}
- return strings.TrimPrefix(parsed.Host, "matrix."), nil
+ host := parsed.Hostname()
+ if host == "" {
+ return "", fmt.Errorf("desktop account homeserver URL %q has no host", account.Homeserver)
+ }
+ return strings.TrimPrefix(host, "matrix."), nil
}📝 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.
| parsed, err := url.Parse(account.Homeserver) | |
| if err != nil { | |
| return "", fmt.Errorf("desktop account has invalid homeserver URL %q: %w", account.Homeserver, err) | |
| } | |
| return strings.TrimPrefix(parsed.Host, "matrix."), nil | |
| parsed, err := url.Parse(account.Homeserver) | |
| if err != nil { | |
| return "", fmt.Errorf("desktop account has invalid homeserver URL %q: %w", account.Homeserver, err) | |
| } | |
| host := parsed.Hostname() | |
| if host == "" { | |
| return "", fmt.Errorf("desktop account homeserver URL %q has no host", account.Homeserver) | |
| } | |
| return strings.TrimPrefix(host, "matrix."), nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/bbctl/desktopauth.go` around lines 136 - 140, The code uses parsed.Host
which can include a port and leads to incorrect envForHomeserverDomain matching;
update the logic that derives the homeserver domain (the block working with
parsed, err := url.Parse(account.Homeserver)) to call parsed.Hostname() instead
of parsed.Host, explicitly reject empty parsed.Hostname() with an error (e.g.,
"empty host in homeserver URL"), and keep the existing TrimPrefix call but apply
it to parsed.Hostname() so ports are excluded when returning the domain used for
environment selection.
There was a problem hiding this comment.
beeper hostnames are matrix.beeper.localtest.me, matrix.beeper-dev.com, matrix.beeper-staging.com, matrix.beeper.com
There was a problem hiding this comment.
@batuhan, thanks for the context! Since all Beeper homeservers use standard ports (no :port suffix in those URLs), parsed.Host and parsed.Hostname() are equivalent in practice — there's no real bug here. The suggested change to Hostname() would only matter if a custom non-standard-port homeserver were used, which isn't the case for Beeper's infrastructure.
Feel free to disregard the port concern. The empty-host guard is still a minor defensive improvement, but it's entirely up to you whether it's worth the added lines.
(´・ω・`) ～ curious bunny learns new things
✏️ Learnings added
Learnt from: batuhan
Repo: beeper/bridge-manager PR: 75
File: cmd/bbctl/desktopauth.go:136-140
Timestamp: 2026-05-04T13:43:17.841Z
Learning: Beeper homeserver hostnames used in bridge-manager/bbctl are: matrix.beeper.localtest.me, matrix.beeper-dev.com, matrix.beeper-staging.com, and matrix.beeper.com. None include non-standard ports, so parsed.Host and parsed.Hostname() are equivalent in desktopAccountHomeserverDomain in cmd/bbctl/desktopauth.go.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
It looks like you're not a member of this organization on Indent. To use Indent, visit app.indent.com to get started. |
Co-authored-by: Tulir Asokan <tulir@maunium.net>
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 `@cmd/bbctl/desktopauth.go`:
- Line 11: The import of the package "runtime" in cmd/bbctl/desktopauth.go is
unused and causes build failures; remove "runtime" from the import list in that
file's import block so the compiler/typechecker no longer errors (no other code
in functions like any init, main, or functions in desktopauth.go reference
runtime).
🪄 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: CHILL
Plan: Pro
Run ID: 93f5a8f1-530f-4ccf-87c0-4d95ba4cb696
📒 Files selected for processing (1)
cmd/bbctl/desktopauth.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: build-docker (arm64)
🧰 Additional context used
🪛 GitHub Check: build
cmd/bbctl/desktopauth.go
[failure] 11-11:
"runtime" imported and not used
[failure] 11-11:
"runtime" imported and not used
[failure] 11-11:
"runtime" imported and not used
[failure] 11-11:
"runtime" imported and not used
🪛 golangci-lint (2.11.4)
cmd/bbctl/desktopauth.go
[error] 11-11: : # github.com/beeper/bridge-manager/cmd/bbctl
cmd/bbctl/desktopauth.go:11:2: "runtime" imported and not used
(typecheck)
Delete the login-desktop CLI command and its handler, and stop registering it with the app. Update prepareApp to use a new isRecoveryCommand check (renamed from isLoginCommand) so desktop credentials are skipped for recovery-related commands; adjust the command list so "login-desktop" is no longer treated as a login case and add "logout" to the recovery set. Kept desktopLoginFlags and loadDesktopLogin for other flows.
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 `@cmd/bbctl/desktopauth.go`:
- Around line 69-100: The file URI built in readDesktopAccount uses dbPath
directly which can contain Windows backslashes, producing an invalid SQLite file
URI; normalize the path by converting dbPath to forward slashes before
constructing dbURI (use filepath.ToSlash on the dbPath) so the url.URL Path uses
forward slashes and the resulting dbURI works on Windows; update the dbURI
construction in readDesktopAccount accordingly.
- Around line 102-110: desktopAccountHomeserverDomain currently strips the
scheme and returns only the host, and callers (configureDesktopLogin and
loadDesktopLogin) use that result as the fallback which causes Whoami to target
the wrong API URL; update the logic so that when no mapped environment is found
the original account.Homeserver (the full URL including scheme) is used for
verification instead of the value returned by desktopAccountHomeserverDomain, or
alternatively change desktopAccountHomeserverDomain to return the full original
URL (or an explicit second return value) so configureDesktopLogin and
loadDesktopLogin can fall back to account.Homeserver for Whoami; ensure you
reference the DesktopAccount.Homeserver field in these functions when falling
back.
🪄 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: CHILL
Plan: Pro
Run ID: f151c818-c7c4-4387-9a80-c227a715602a
📒 Files selected for processing (2)
cmd/bbctl/desktopauth.gocmd/bbctl/main.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/bbctl/main.go
📜 Review details
🔇 Additional comments (1)
cmd/bbctl/desktopauth.go (1)
21-67: Looks good.The flag plumbing and desktop-data-dir resolution are cleanly separated and easy to follow.
2 participants
Summary
bbctl login-desktopto import Beeper Desktop Matrix credentials into bbctl configbbctl loginoffer the existing Desktop account before falling back to email-code loginBEEPER_PROFILEconvention as Desktopsyt_Matrix access tokens by treating any non-empty configured token as credentialsValidation
go test ./...