Slskd integration#54
Conversation
|
Thank you for your work @paulo-roger, I will review as soon as possible. |
|
@paulo-roger — first of all, thank you. This is a serious piece of work, and the architecture genuinely fits the data-driven seams the codebase was designed around ( I'd like to merge this once a few items are addressed. Splitting them by severity: 🚨 Blocking — security1. Chat/room mutation routes are gated on the read-only
Any logged-in user with 2. No per-user isolation on DMs. There's one slskd account per instance, and the API doesn't consult 🛠 Should fix — architecture conventions3. Logger usage in the two new API classes. Both
Drop the bracket prefix and call 4.
The data-driven goal of the refactor is half-done — adding a 4th network later means edits in three places. Could you consolidate into a single 5. 📌 Nits — non-blocking, your call
✅ What you got right (so you know what NOT to change)
Suggested order
Happy to clarify any of this. Once 1–5 are in I'm ready to merge. Thanks again for the heavy lift on this! |
|
Two issues from manual testing — both have concrete fixes from existing patterns in the codebase. 1. Add-client modal: auth dropdownRight now The rtorrent entry at slskd: [
{ field: 'host', label: 'Host', ..., required: true },
{ field: 'port', label: 'Port', ..., required: true, parseValue: v => parseInt(v, 10) || 5030 },
{ field: 'path', label: 'URL Path (Optional)', ... },
{ field: 'authMode', label: 'Authentication', select: true,
options: [
{ value: 'apiKey', label: 'API Key (Recommended)' },
{ value: 'userpass', label: 'Username & Password' }
],
defaultValue: 'apiKey' },
{ field: 'apiKey', label: 'API Key', ..., required: true, sensitive: true,
hideWhen: form => (form.authMode || 'apiKey') !== 'apiKey' },
{ field: 'username', label: 'Username', ..., required: true,
hideWhen: form => (form.authMode || 'apiKey') !== 'userpass' },
{ field: 'password', label: 'Password', ..., required: true, sensitive: true,
hideWhen: form => (form.authMode || 'apiKey') !== 'userpass' },
{ field: 'useSsl', ..., toggle: true },
{ field: 'downloadDirectory', ... }
]The 2. Auto-reconnect not working + stale connection handler persists after disable/re-enableTwo interleaved bugs here: 2a. Fix: add one line to async cleanup() {
this.clearReconnect(); // ← add this
if (this.client) {
await this.client.disconnect();
this.client = null;
}
this.releaseSearchLock();
}Compare to 2b. Compare to Refactor suggestion: move the connection-test + success-handling logic from async initClient() {
if (!this._clientConfig?.enabled) {
this.log('slskd integration is disabled');
return false;
}
if (!this._clientConfig.host) {
this.log('slskd host not configured');
return false;
}
const newClient = new SlskdClient({ /* ...config */ });
try {
const test = await newClient.testConnection();
if (!test.success) throw new Error(test.error || 'Failed to connect');
this.client = newClient;
this._downloadDirectory = this._clientConfig.downloadDirectory || '';
this._clearConnectionError();
this.clearReconnect();
this.log('Connected to slskd successfully');
this._onConnectCallbacks.forEach(cb => cb());
return true;
} catch (err) {
this.error('Failed to connect to slskd:', logger.errorDetail(err));
this._setConnectionError(err);
await newClient.disconnect().catch(() => {});
this.client = null;
return false;
}
}
async startConnection() {
const connected = await this.initClient();
if (!connected && this._clientConfig?.enabled) {
this.scheduleReconnect(30000);
}
}With this shape, the retry interval from
These two changes together should make slskd behave the same as qbittorrent for both the "remote crashed mid-session" and "user toggled in settings" flows. |
|
Thank you very much for all the analysis and feedback, I will do as you say and correct everything. I still have a lot to learn so I really appreciate your feedback! |
2 participants
Full Soulseek integration via slskd:
Backend
/api/v0/*endpoints; supportsX-API-Keyheader auth (recommended) and JWT fallback via username/password; auto-retries on 401; robust nested-object search-result extraction to handle slskd's variable response shapes.SlskdManagerextendingBaseClientManager: connection lifecycle with 30s reconnect back-off, download/upload polling, search, shared-file tracking, stats aggregation, andonConnectSyncto register the download directory withCategoryManager.slskdclient-type metadata entry.normalizeSlskdDownloadandnormalizeSlskdSharedFile.Frontend
dataSlskdLogsstate +slskd-log-updateWS event handler.useSearchProviderSelector.Some changes to better accommodate a new network type: Soulseek
These changes collectively remove hardcoded assumptions about exactly two network types (eD2K and BitTorrent) across the front‑end codebase. The original implementation duplicated logic, props, and markup for each network, making the addition of a third network (Soulseek) brittle and labour‑intensive.
The refactoring introduces a generic, data‑driven approach:
NETWORK_CONFIGSarray defines all supported network types, their labels, icons, colors, and behaviours. UI elements that previously hardcoded “ED2K” and “BT” now iterate over this array.StatsWidget,Header,StatisticsView, and table columns have been rewritten to accept dynamic lists of active networks rather than fixed pairs of booleans or separate props. Helper functions (getStatValue,renderNetworkToggleButton,renderInstanceGroup,ClientBreakdownValue, etc.) use the generic configuration to render the correct set of controls and data for however many networks are enabled.NETWORK_CONFIGSand, where relevant, the corresponding API data. No further JSX or component logic changes are needed. The same pattern will accommodate future network types with zero duplication.The immediate motivation is to integrate Soulseek support
Sorry if I am missing something, you can ask anything. I am without sleeping for too many hours already, took me a lot of time, really!
I really appreciate all you did, thanks!