Skip to content

Commit dc7fefc

Browse files
Owie6789Owie6789
committed
fix(Sandakan#498): address CodeRabbit review findings (smart playlists + lastfm)
playlist-rules.ts - numeric helper now returns undefined for !Number.isFinite(n), guarding against NaN/Infinity values from malformed rule inputs - playCount / skipCount switches now handle 'neq' operator (was silently dropped before) - lastPlayed switch now handles 'neq' (via not-between on the same window) - All three branches guard against !Number.isFinite(value) before the switch getUserLovedTracks.ts / getUserRecentTracks.ts / getUserTopTracks.ts - URL changed http -> https - Added AbortController-based 10s timeout around fetch - Non-OK response now throws Error with status+statusText instead of returning undefined (errors are now visible to callers) - Catch block re-throws so the IPC layer can surface them ipc.ts - Last.fm IPC handlers now validate inputs at the boundary: trim username, reject empty, clamp limit to [1,100], validate period against allowed set - app/refreshSmartPlaylist JSON.parse is now wrapped in try/catch with a safe { songIds: [] } fallback and logger.error schema.ts - Added idx_playlists_is_smart Drizzle index to match the 0000_add_smart_playlist_columns.sql migration (closes schema drift) SmartPlaylistCriteriaEditor.tsx - Added isValidCriteriaShape type guard: parsed JSON must have matchType ALL/ANY and rules[] with string field/operator - placeholder='value' replaced with t('playlist.criteriaValuePlaceholder') - saveCriteria now wraps the IPC call in try/catch and notifies on error (previously unhandled rejection) en.json - Added playlist.criteriaValuePlaceholder = 'value'
1 parent a5f8275 commit dc7fefc

8 files changed

Lines changed: 153 additions & 63 deletions

File tree

‎src/main/db/queries/playlist-rules.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ function buildCondition(rule: SmartPlaylistRule): SQL | undefined {
88

99
const numeric = (col: SQL) => {
1010
const n = Number(value);
11+
if (!Number.isFinite(n)) return undefined;
1112
switch (operator) {
1213
case 'eq': return sql`${col} = ${n}`;
1314
case 'neq': return sql`${col} != ${n}`;
@@ -74,10 +75,12 @@ function buildCondition(rule: SmartPlaylistRule): SQL | undefined {
7475

7576
case 'playCount': {
7677
const n = Number(value);
78+
if (!Number.isFinite(n)) return undefined;
7779
switch (operator) {
7880
case 'gt': return sql`(select count(*) from play_events where play_events.song_id = songs.id) > ${n}`;
7981
case 'gte': return sql`(select count(*) from play_events where play_events.song_id = songs.id) >= ${n}`;
8082
case 'eq': return sql`(select count(*) from play_events where play_events.song_id = songs.id) = ${n}`;
83+
case 'neq': return sql`(select count(*) from play_events where play_events.song_id = songs.id) != ${n}`;
8184
case 'lt': return sql`(select count(*) from play_events where play_events.song_id = songs.id) < ${n}`;
8285
case 'lte': return sql`(select count(*) from play_events where play_events.song_id = songs.id) <= ${n}`;
8386
default: return undefined;
@@ -86,10 +89,12 @@ function buildCondition(rule: SmartPlaylistRule): SQL | undefined {
8689

8790
case 'skipCount': {
8891
const n = Number(value);
92+
if (!Number.isFinite(n)) return undefined;
8993
switch (operator) {
9094
case 'gt': return sql`(select count(*) from skip_events where skip_events.song_id = songs.id) > ${n}`;
9195
case 'gte': return sql`(select count(*) from skip_events where skip_events.song_id = songs.id) >= ${n}`;
9296
case 'eq': return sql`(select count(*) from skip_events where skip_events.song_id = songs.id) = ${n}`;
97+
case 'neq': return sql`(select count(*) from skip_events where skip_events.song_id = songs.id) != ${n}`;
9398
case 'lt': return sql`(select count(*) from skip_events where skip_events.song_id = songs.id) < ${n}`;
9499
case 'lte': return sql`(select count(*) from skip_events where skip_events.song_id = songs.id) <= ${n}`;
95100
default: return undefined;
@@ -98,12 +103,14 @@ function buildCondition(rule: SmartPlaylistRule): SQL | undefined {
98103

99104
case 'lastPlayed': {
100105
const daysAgo = Number(value);
106+
if (!Number.isFinite(daysAgo)) return undefined;
101107
const cutoff = new Date(Date.now() - daysAgo * 24 * 60 * 60 * 1000);
102108
const lastPlayed = sql`(select max(play_history.created_at) from play_history where play_history.song_id = songs.id)`;
103109
switch (operator) {
104110
case 'gt': return sql`${lastPlayed} > ${cutoff}::timestamp`;
105111
case 'gte': return sql`${lastPlayed} >= ${cutoff}::timestamp`;
106112
case 'eq': return sql`${lastPlayed} between ${cutoff}::timestamp and ${new Date(cutoff.getTime() + 86400000)}::timestamp`;
113+
case 'neq': return sql`${lastPlayed} not between ${cutoff}::timestamp and ${new Date(cutoff.getTime() + 86400000)}::timestamp`;
107114
case 'lt': return sql`${lastPlayed} < ${cutoff}::timestamp`;
108115
case 'lte': return sql`${lastPlayed} <= ${cutoff}::timestamp`;
109116
default: return undefined;

‎src/main/db/schema.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,9 @@ export const playlists = pgTable(
313313
// GIN index for fuzzy matching with pg_trgm trigram operator
314314
index('idx_playlists_name_ci_trgm').using('gin', t.nameCI.op('gin_trgm_ops')),
315315
// Index for creation date sorting
316-
index('idx_playlists_created_at').on(t.createdAt.desc())
316+
index('idx_playlists_created_at').on(t.createdAt.desc()),
317+
// Index for filtering smart playlists (matches the SQL in 0000_add_smart_playlist_columns.sql)
318+
index('idx_playlists_is_smart').on(t.isSmart)
317319
]
318320
);
319321

‎src/main/ipc.ts‎

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -337,24 +337,50 @@ export function initializeIPC(mainWindow: BrowserWindow, abortSignal: AbortSigna
337337
getAlbumInfoFromLastFM(albumId)
338338
);
339339

340+
const allowedPeriods = new Set([
341+
'overall',
342+
'7day',
343+
'1month',
344+
'3month',
345+
'6month',
346+
'12month'
347+
]);
348+
const clampLimit = (n: number | undefined): number => {
349+
if (typeof n !== 'number' || !Number.isFinite(n)) return 50;
350+
return Math.min(100, Math.max(1, Math.floor(n)));
351+
};
352+
340353
ipcMain.handle(
341354
'app/lastfm/getUserTopTracks',
342355
(
343356
_,
344357
username: string,
345358
period?: 'overall' | '7day' | '1month' | '3month' | '6month' | '12month',
346359
limit?: number
347-
) => getUserTopTracks(username, period, limit)
360+
) => {
361+
const cleanUser = (username ?? '').trim();
362+
if (!cleanUser) return undefined;
363+
const cleanPeriod = period && allowedPeriods.has(period) ? period : 'overall';
364+
return getUserTopTracks(cleanUser, cleanPeriod, clampLimit(limit));
365+
}
348366
);
349367

350368
ipcMain.handle(
351369
'app/lastfm/getUserRecentTracks',
352-
(_, username: string, limit?: number) => getUserRecentTracks(username, limit)
370+
(_, username: string, limit?: number) => {
371+
const cleanUser = (username ?? '').trim();
372+
if (!cleanUser) return undefined;
373+
return getUserRecentTracks(cleanUser, clampLimit(limit));
374+
}
353375
);
354376

355377
ipcMain.handle(
356378
'app/lastfm/getUserLovedTracks',
357-
(_, username: string, limit?: number) => getUserLovedTracks(username, limit)
379+
(_, username: string, limit?: number) => {
380+
const cleanUser = (username ?? '').trim();
381+
if (!cleanUser) return undefined;
382+
return getUserLovedTracks(cleanUser, clampLimit(limit));
383+
}
358384
);
359385

360386
ipcMain.handle('app/getSongListeningData', (_, songIds: number[]) => getListeningData(songIds));
@@ -487,7 +513,13 @@ export function initializeIPC(mainWindow: BrowserWindow, abortSignal: AbortSigna
487513
ipcMain.handle('app/refreshSmartPlaylist', async (_, playlistId: number) => {
488514
const playlist = await getPlaylistById(playlistId);
489515
if (!playlist?.criteria) return { songIds: [] };
490-
const criteria: SmartPlaylistCriteria = JSON.parse(playlist.criteria);
516+
let criteria: SmartPlaylistCriteria;
517+
try {
518+
criteria = JSON.parse(playlist.criteria) as SmartPlaylistCriteria;
519+
} catch (error) {
520+
logger.error('Invalid smart playlist criteria JSON', { playlistId, error });
521+
return { songIds: [] };
522+
}
491523
const songIds = await refreshSmartPlaylist(playlistId, criteria);
492524
return { songIds };
493525
});

‎src/main/other/lastFm/getUserLovedTracks.ts‎

Lines changed: 23 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -22,31 +22,38 @@ const getUserLovedTracks = async (
2222
const isOnline = checkIfConnectedToInternet();
2323
if (!isOnline) throw new Error('App not connected to internet.');
2424

25-
const url = new URL('http://ws.audioscrobbler.com/2.0/');
25+
const url = new URL('https://ws.audioscrobbler.com/2.0/');
2626
url.searchParams.set('method', 'user.getLovedTracks');
2727
url.searchParams.set('api_key', LAST_FM_API_KEY);
2828
url.searchParams.set('user', username);
2929
url.searchParams.set('limit', String(limit));
3030
url.searchParams.set('format', 'json');
3131

32-
const res = await fetch(url);
33-
if (res.ok) {
34-
const data = await res.json();
35-
if (data.error) throw new Error(`${data.error} - ${data.message}`);
36-
37-
const lovedTracks = data.lovedtracks?.track ?? [];
38-
return {
39-
tracks: lovedTracks.map((track: { name: string; url: string; artist: { name: string } }) => ({
40-
name: track.name,
41-
artist: track.artist.name,
42-
url: track.url
43-
}))
44-
};
32+
const controller = new AbortController();
33+
const timer = setTimeout(() => controller.abort(), 10_000);
34+
let res: Response;
35+
try {
36+
res = await fetch(url, { signal: controller.signal });
37+
} finally {
38+
clearTimeout(timer);
4539
}
46-
return undefined;
40+
if (!res.ok) {
41+
throw new Error(`LastFM user.getLovedTracks returned ${res.status} ${res.statusText}`);
42+
}
43+
const data = await res.json();
44+
if (data.error) throw new Error(`${data.error} - ${data.message}`);
45+
46+
const lovedTracks = data.lovedtracks?.track ?? [];
47+
return {
48+
tracks: lovedTracks.map((track: { name: string; url: string; artist: { name: string } }) => ({
49+
name: track.name,
50+
artist: track.artist.name,
51+
url: track.url
52+
}))
53+
};
4754
} catch (error) {
4855
logger.error('Failed to get loved tracks from LastFM.', { error });
49-
return undefined;
56+
throw error;
5057
}
5158
};
5259

‎src/main/other/lastFm/getUserRecentTracks.ts‎

Lines changed: 24 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -23,32 +23,39 @@ const getUserRecentTracks = async (
2323
const isOnline = checkIfConnectedToInternet();
2424
if (!isOnline) throw new Error('App not connected to internet.');
2525

26-
const url = new URL('http://ws.audioscrobbler.com/2.0/');
26+
const url = new URL('https://ws.audioscrobbler.com/2.0/');
2727
url.searchParams.set('method', 'user.getRecentTracks');
2828
url.searchParams.set('api_key', LAST_FM_API_KEY);
2929
url.searchParams.set('user', username);
3030
url.searchParams.set('limit', String(limit));
3131
url.searchParams.set('format', 'json');
3232

33-
const res = await fetch(url);
34-
if (res.ok) {
35-
const data = await res.json();
36-
if (data.error) throw new Error(`${data.error} - ${data.message}`);
37-
38-
const recentTracks = data.recenttracks?.track ?? [];
39-
return {
40-
tracks: recentTracks.map((track: { name: string; url: string; artist: { '#text': string }; date?: { uts: string } }) => ({
41-
name: track.name,
42-
artist: track.artist['#text'],
43-
url: track.url,
44-
playedAt: track.date ? Number(track.date.uts) : 0
45-
}))
46-
};
33+
const controller = new AbortController();
34+
const timer = setTimeout(() => controller.abort(), 10_000);
35+
let res: Response;
36+
try {
37+
res = await fetch(url, { signal: controller.signal });
38+
} finally {
39+
clearTimeout(timer);
4740
}
48-
return undefined;
41+
if (!res.ok) {
42+
throw new Error(`LastFM user.getRecentTracks returned ${res.status} ${res.statusText}`);
43+
}
44+
const data = await res.json();
45+
if (data.error) throw new Error(`${data.error} - ${data.message}`);
46+
47+
const recentTracks = data.recenttracks?.track ?? [];
48+
return {
49+
tracks: recentTracks.map((track: { name: string; url: string; artist: { '#text': string }; date?: { uts: string } }) => ({
50+
name: track.name,
51+
artist: track.artist['#text'],
52+
url: track.url,
53+
playedAt: track.date ? Number(track.date.uts) : 0
54+
}))
55+
};
4956
} catch (error) {
5057
logger.error('Failed to get recent tracks from LastFM.', { error });
51-
return undefined;
58+
throw error;
5259
}
5360
};
5461

‎src/main/other/lastFm/getUserTopTracks.ts‎

Lines changed: 24 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -24,33 +24,40 @@ const getUserTopTracks = async (
2424
const isOnline = checkIfConnectedToInternet();
2525
if (!isOnline) throw new Error('App not connected to internet.');
2626

27-
const url = new URL('http://ws.audioscrobbler.com/2.0/');
27+
const url = new URL('https://ws.audioscrobbler.com/2.0/');
2828
url.searchParams.set('method', 'user.getTopTracks');
2929
url.searchParams.set('api_key', LAST_FM_API_KEY);
3030
url.searchParams.set('user', username);
3131
url.searchParams.set('period', period);
3232
url.searchParams.set('limit', String(limit));
3333
url.searchParams.set('format', 'json');
3434

35-
const res = await fetch(url);
36-
if (res.ok) {
37-
const data = await res.json();
38-
if (data.error) throw new Error(`${data.error} - ${data.message}`);
39-
40-
const topTracks = data.toptracks?.track ?? [];
41-
return {
42-
tracks: topTracks.map((track: { name: string; url: string; artist: { name: string }; playcount: string }) => ({
43-
name: track.name,
44-
artist: track.artist.name,
45-
url: track.url,
46-
playCount: Number(track.playcount)
47-
}))
48-
};
35+
const controller = new AbortController();
36+
const timer = setTimeout(() => controller.abort(), 10_000);
37+
let res: Response;
38+
try {
39+
res = await fetch(url, { signal: controller.signal });
40+
} finally {
41+
clearTimeout(timer);
4942
}
50-
return undefined;
43+
if (!res.ok) {
44+
throw new Error(`LastFM user.getTopTracks returned ${res.status} ${res.statusText}`);
45+
}
46+
const data = await res.json();
47+
if (data.error) throw new Error(`${data.error} - ${data.message}`);
48+
49+
const topTracks = data.toptracks?.track ?? [];
50+
return {
51+
tracks: topTracks.map((track: { name: string; url: string; artist: { name: string }; playcount: string }) => ({
52+
name: track.name,
53+
artist: track.artist.name,
54+
url: track.url,
55+
playCount: Number(track.playcount)
56+
}))
57+
};
5158
} catch (error) {
5259
logger.error('Failed to get top tracks from LastFM.', { error });
53-
return undefined;
60+
throw error;
5461
}
5562
};
5663

‎src/renderer/src/assets/locales/en/en.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,7 @@
295295
"operatorContains": "contains",
296296
"criteriaSaveSuccess": "Smart playlist criteria saved successfully.",
297297
"criteriaSaveFailed": "Failed to save smart playlist criteria.",
298+
"criteriaValuePlaceholder": "value",
298299
"refreshSuccess": "Smart playlist refreshed.",
299300
"refreshFailed": "Failed to refresh smart playlist.",
300301
"ruleField": "Field",

‎src/renderer/src/components/SmartPlaylistCriteriaEditor.tsx‎

Lines changed: 35 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,20 @@ function defaultCriteria(): SmartPlaylistCriteria {
3030
return { matchType: 'ALL', rules: [defaultRule()] };
3131
}
3232

33+
function isValidCriteriaShape(value: unknown): value is SmartPlaylistCriteria {
34+
if (!value || typeof value !== 'object') return false;
35+
const v = value as Partial<SmartPlaylistCriteria>;
36+
if (v.matchType !== 'ALL' && v.matchType !== 'ANY') return false;
37+
if (!Array.isArray(v.rules)) return false;
38+
return v.rules.every(
39+
(r) =>
40+
r &&
41+
typeof r === 'object' &&
42+
typeof (r as SmartPlaylistRule).field === 'string' &&
43+
typeof (r as SmartPlaylistRule).operator === 'string'
44+
);
45+
}
46+
3347
const SmartPlaylistCriteriaEditor = (props: SmartPlaylistCriteriaEditorProps) => {
3448
const { playlist } = props;
3549
const { t } = useTranslation();
@@ -38,9 +52,10 @@ const SmartPlaylistCriteriaEditor = (props: SmartPlaylistCriteriaEditorProps) =>
3852
const initialCriteria = useMemo(() => {
3953
if (playlist.criteria) {
4054
try {
41-
return JSON.parse(playlist.criteria) as SmartPlaylistCriteria;
55+
const parsed = JSON.parse(playlist.criteria);
56+
if (isValidCriteriaShape(parsed)) return parsed;
4257
} catch {
43-
return defaultCriteria();
58+
// fall through to default below
4459
}
4560
}
4661
return defaultCriteria();
@@ -122,10 +137,22 @@ const SmartPlaylistCriteriaEditor = (props: SmartPlaylistCriteriaEditorProps) =>
122137
}
123138
const cleaned: SmartPlaylistCriteria = { ...criteria, rules: cleanRules };
124139

125-
const result = await window.api.playlistsData.saveSmartPlaylistCriteria(
126-
playlist.playlistId,
127-
cleaned
128-
);
140+
const result = await window.api.playlistsData
141+
.saveSmartPlaylistCriteria(playlist.playlistId, cleaned)
142+
.catch((error: unknown) => {
143+
addNewNotifications([
144+
{
145+
id: 'smartPlaylistSaveFailed',
146+
duration: 5000,
147+
content: t('playlist.criteriaSaveFailed')
148+
}
149+
]);
150+
// Re-throw is intentionally suppressed: the failure has been
151+
// surfaced to the user. The catch+return-false pattern keeps
152+
// the rest of the editor in a known state.
153+
void error;
154+
return false as const;
155+
});
129156
if (result) {
130157
addNewNotifications([
131158
{
@@ -177,7 +204,7 @@ const SmartPlaylistCriteriaEditor = (props: SmartPlaylistCriteriaEditorProps) =>
177204
className="mt-1 w-full rounded-lg bg-background-color-2 px-3 py-1.5 text-sm text-font-color-black outline-1 outline-transparent transition-colors focus:outline-font-color-highlight dark:bg-dark-background-color-2 dark:text-font-color-white dark:focus:outline-dark-font-color-highlight"
178205
value={String(rule.value)}
179206
onChange={(e) => onChange(e.target.value)}
180-
placeholder="value"
207+
placeholder={t('playlist.criteriaValuePlaceholder')}
181208
/>
182209
);
183210
}
@@ -188,7 +215,7 @@ const SmartPlaylistCriteriaEditor = (props: SmartPlaylistCriteriaEditorProps) =>
188215
className="mt-1 w-full rounded-lg bg-background-color-2 px-3 py-1.5 text-sm text-font-color-black outline-1 outline-transparent transition-colors focus:outline-font-color-highlight dark:bg-dark-background-color-2 dark:text-font-color-white dark:focus:outline-dark-font-color-highlight"
189216
value={String(rule.value)}
190217
onChange={(e) => onChange(Number(e.target.value))}
191-
placeholder="value"
218+
placeholder={t('playlist.criteriaValuePlaceholder')}
192219
/>
193220
);
194221
};

0 commit comments

Comments
 (0)