feat: add node alias setting to General settings page#2039
feat: add node alias setting to General settings page#2039sat-engineer wants to merge 1 commit intogetAlby:masterfrom
Conversation
📝 WalkthroughWalkthroughThe PR significantly expands the public API surface across multiple layers: introducing comprehensive new data types and request/response structures for apps, swaps, transactions, and node operations; enhancing the LNClient interface with extensive new method signatures and model types for channels, balances, and custom commands; extending the Config interface with additional management methods; and adding frontend UI support for setting node aliases. Additionally, test coverage is added for the new node alias functionality. Changes
Poem
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
Add inline node alias editing on the Settings > General page so self-hosted users can change their Lightning node alias without needing an Alby paid plan. The alias input pre-populates from useInfo().nodeAlias and POSTs to /api/node/alias. A toast reminds the user to restart for the change to take effect. Also adds Go doc comments to exported types in api/models.go, config/models.go, and lnclient/models.go to satisfy the 80% docstring coverage threshold. Includes tests for SetNodeAlias API covering success, empty alias, and config error scenarios. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sat-engineer
force-pushed
the
feat/node-alias-settings
branch
from
4cafde5 to
6da3bbe
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tor/tor.go`:
- Around line 155-161: In Start(), if the concurrent-start branch detects
s.running true, ensure the onion created earlier is removed before closing the
control connection: call ctrl.DelOnion(resp.ServiceID) (checking resp != nil and
handling/logging any returned error) prior to ctrl.Close(); this will prevent
leaving an orphaned hidden service when the losing goroutine exits. Use the
existing resp variable from the AddOnion call and keep the existing ctrl.Close()
cleanup after attempting DelOnion.
🧹 Nitpick comments (7)
lnclient/models.go (1)
202-203: Nit: missing blank line betweenPeerDetailsandLightningBalanceResponse.There's a missing blank line separator between the closing brace of
PeerDetails(line 202) and the doc comment forLightningBalanceResponse(line 203), which is inconsistent with the rest of the file.Proposed fix
} + // LightningBalanceResponse contains the Lightning channel balance breakdown. type LightningBalanceResponse struct {api/api.go (1)
1251-1252: BackendTorEnabledlogic is correct; consider conditional frontend menu visibility for consistency.The backend correctly gates
TorEnabledon bothLDKBackendTypeandLDKTorEnabled. However, the Tor menu item inSettingsLayout.tsx(lines 121-123) is shown for all LDK backends (info?.backendType === "LDK"), not conditionally oninfo.torEnabled.The
TorSettings.tsxcomponent already handles both states—showing a disabled message whentorEnabledis false and settings when true. For frontend consistency with the backend gating logic, consider usinginfo?.torEnabledas the menu visibility condition instead of checking onlybackendType. This would prevent showing the Tor menu item for LDK backends that don't have Tor configured.frontend/src/screens/settings/Settings.tsx (2)
43-47: Nit: Use the already-destructureduseEffectinstead ofReact.useEffect.Line 2 destructures
useEffectfrom React, but line 43 usesReact.useEffect. Use the destructured version for consistency with the rest of the file.♻️ Suggested fix
- React.useEffect(() => { + useEffect(() => { if (info?.nodeAlias !== undefined) { setNodeAlias(info.nodeAlias); } }, [info?.nodeAlias]);
261-268: AddmaxLength={32}to the node alias input to prevent user confusion.Lightning node aliases are limited to 32 bytes by the BOLT
#7protocol specification. Without client-side validation, users could enter a longer string that may be silently truncated or handled unexpectedly by the backend.🛡️ Suggested fix
<Input id="nodeAlias" type="text" value={nodeAlias} onChange={(e) => setNodeAlias(e.target.value)} placeholder="Alby Hub" className="w-full md:w-60" + maxLength={32} />lnclient/ldk/ldk.go (2)
353-383: Tor hidden service start: soft-fail is reasonable, but verify the listening port parse.The Tor startup continues on error (line 378), which aligns with the design intent. However, the listening-port parsing at lines 358–364 silently falls back to
DefaultOnionServicePortif the address format is unexpected orLDKListeningAddressesis blank. If the LDK node actually listens on a non-default port, the onion service would forward to the wrong port.Consider logging a warning when the port parse fails to fall back to the default, so operators can diagnose connectivity issues.
🔍 Suggested diagnostic logging
if addrs := strings.Split(cfg.GetEnv().LDKListeningAddresses, ","); len(addrs) > 0 { addr := strings.TrimSpace(addrs[0]) if idx := strings.LastIndex(addr, ":"); idx >= 0 { if p, err := strconv.Atoi(addr[idx+1:]); err == nil { localPort = p + } else { + logger.Logger.WithError(err).WithField("addr", addr).Warn("Failed to parse LDK listening port for Tor, using default") } } }
936-942: Optional: Extract alias resolution to a small helper to reduce duplication.The same
cfg.Get("NodeAlias", "")+ default-to-"Alby Hub"pattern appears both here and inNewLDKService(lines 137–140). A tiny helper (e.g.,func (ls *LDKService) getNodeAlias() string) would centralize the default.api/models.go (1)
354-358: Nit: Missing blank line betweenChangeUnlockPasswordRequestandAutoUnlockRequest.Go convention places a blank line between type declarations (especially when each has a doc comment). Same issue between
RebalanceChannelRequestandRebalanceChannelResponseat lines 379–383.♻️ Suggested fix
type ChangeUnlockPasswordRequest struct { CurrentUnlockPassword string `json:"currentUnlockPassword"` NewUnlockPassword string `json:"newUnlockPassword"` } + // AutoUnlockRequest contains the password used for automatic unlock on startup. type AutoUnlockRequest struct { UnlockPassword string `json:"unlockPassword"` }type RebalanceChannelRequest struct { ReceiveThroughNodePubkey string `json:"receiveThroughNodePubkey"` AmountSat uint64 `json:"amountSat"` } + // RebalanceChannelResponse contains the fee paid for the channel rebalance. type RebalanceChannelResponse struct { TotalFeeSat uint64 `json:"totalFeeSat"` }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lnclient/models.go`:
- Around line 58-63: The struct comment for NodeConnectionInfo incorrectly
mentions "including optional Tor address" even though the struct only defines
Pubkey, Address, and Port; update the doc comment above the NodeConnectionInfo
type to accurately describe the current fields (Pubkey, Address, Port) and
remove the Tor reference, or if Tor support is intended now, add a TorAddress
(or Onion) field to NodeConnectionInfo and document it accordingly; ensure
references to NodeConnectionInfo, Pubkey, Address, and Port are consistent.
🧹 Nitpick comments (3)
frontend/src/screens/settings/Settings.tsx (1)
43-47: Inconsistent hook usage:React.useEffectvs. bareuseEffect.Line 43 uses
React.useEffectwhile line 49 usesuseEffect(the named import from line 2). Pick one style — sinceuseEffectanduseStateare already destructured in the import, use the bare form here too.♻️ Suggested fix
- React.useEffect(() => { + useEffect(() => { if (info?.nodeAlias !== undefined) { setNodeAlias(info.nodeAlias); } }, [info?.nodeAlias]);lnclient/models.go (1)
200-201: Missing blank line beforeLightningBalanceResponsedoc comment.Every other type in this file has a blank line separating it from the preceding type. This one is missing it after the
PeerDetailsclosing brace.♻️ Suggested fix
type PeerDetails struct { NodeId string `json:"nodeId"` Address string `json:"address"` IsPersisted bool `json:"isPersisted"` IsConnected bool `json:"isConnected"` } + // LightningBalanceResponse contains the Lightning channel balance breakdown. type LightningBalanceResponse struct {api/models.go (1)
353-357: Missing blank line beforeAutoUnlockRequestdoc comment.Same formatting inconsistency as in
lnclient/models.go— the rest of the file separates types with blank lines.♻️ Suggested fix
type ChangeUnlockPasswordRequest struct { CurrentUnlockPassword string `json:"currentUnlockPassword"` NewUnlockPassword string `json:"newUnlockPassword"` } + // AutoUnlockRequest contains the password used for automatic unlock on startup. type AutoUnlockRequest struct {
| // NodeConnectionInfo contains the connection details for a Lightning node, including optional Tor address. | ||
| type NodeConnectionInfo struct { | ||
| Pubkey string `json:"pubkey"` | ||
| Address string `json:"address"` | ||
| Port int `json:"port"` | ||
| } |
There was a problem hiding this comment.
Doc comment references Tor address that doesn't exist on this struct.
The comment says "including optional Tor address," but NodeConnectionInfo has only Pubkey, Address, and Port. If Tor support is coming in a follow-up PR/commit, keep the doc accurate for the current state of the code.
♻️ Suggested fix
-// NodeConnectionInfo contains the connection details for a Lightning node, including optional Tor address.
+// NodeConnectionInfo contains the connection details for a Lightning node.
type NodeConnectionInfo struct {📝 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.
| // NodeConnectionInfo contains the connection details for a Lightning node, including optional Tor address. | |
| type NodeConnectionInfo struct { | |
| Pubkey string `json:"pubkey"` | |
| Address string `json:"address"` | |
| Port int `json:"port"` | |
| } | |
| // NodeConnectionInfo contains the connection details for a Lightning node. | |
| type NodeConnectionInfo struct { | |
| Pubkey string `json:"pubkey"` | |
| Address string `json:"address"` | |
| Port int `json:"port"` | |
| } |
🤖 Prompt for AI Agents
In `@lnclient/models.go` around lines 58 - 63, The struct comment for
NodeConnectionInfo incorrectly mentions "including optional Tor address" even
though the struct only defines Pubkey, Address, and Port; update the doc comment
above the NodeConnectionInfo type to accurately describe the current fields
(Pubkey, Address, Port) and remove the Tor reference, or if Tor support is
intended now, add a TorAddress (or Onion) field to NodeConnectionInfo and
document it accordingly; ensure references to NodeConnectionInfo, Pubkey,
Address, and Port are consistent.
The node alias settings UI belongs in PR getAlby#2039. Removed Settings.tsx changes to keep this PR focused on the network graph. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Hi, thanks for the PR but we already have a feature to change the node alias. I will close this PR for now. |
3 participants
Summary
useInfo().nodeAlias, POSTs to existing/api/node/aliasendpoint, and shows a restart reminder toastbackendType === "LDK"api/models.go,config/models.go, andlnclient/models.goto satisfy the 80% docstring coverage CI checkTest plan
POST /api/node/aliasis called with correct payload (browser network tab)go test ./...passesDepends on #2038
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes