-
Notifications
You must be signed in to change notification settings - Fork 19
Feat/emote bar #95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Feat/emote bar #95
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Key Issues
The code lacks proper error handling and logging, with several instances of unhandled exceptions and empty catch blocks that could hide critical issues. There is unsafe handling of HTTP responses and type assertions, which could lead to runtime errors and incorrect data processing. The logic for parsing and validating data, such as srcset URLs and channel keys, is flawed and may result in broken functionality or incorrect data usage.
| const url = `https://emotes.adamcy.pl/v1/channel/${encodeURIComponent(username)}/emotes/all`; | ||
| const { data } = await this.httpClient.request<Array<{ provider: number; code: string }>>(url, { timeout: 8000 }); | ||
| const allowed = new Set((data || []).map((d) => d.code)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Possible Bug
The HTTP request on line 188 lacks error handling. If the request fails, it will throw an unhandled exception since there's no try-catch block. This could crash the application when network issues occur or the API is unavailable.
| const url = `https://emotes.adamcy.pl/v1/channel/${encodeURIComponent(username)}/emotes/all`; | |
| const { data } = await this.httpClient.request<Array<{ provider: number; code: string }>>(url, { timeout: 8000 }); | |
| const allowed = new Set((data || []).map((d) => d.code)); | |
| const url = `https://emotes.adamcy.pl/v1/channel/${encodeURIComponent(username)}/emotes/all`; | |
| let data: Array<{ provider: number; code: string }> = []; | |
| try { | |
| const response = await this.httpClient.request<Array<{ provider: number; code: string }>>(url, { timeout: 8000 }); | |
| data = response.data || []; | |
| } catch (error) { | |
| console.error('Failed to fetch channel emotes:', error); | |
| } | |
| const allowed = new Set(data.map((d) => d.code)); |
| const { data } = await this.httpClient.request<Array<{ provider: number; code: string }>>(url, { timeout: 8000 }); | ||
| const allowed = new Set((data || []).map((d) => d.code)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Possible Bug
The code assumes the HTTP response will always have a data property. If the response is malformed or undefined, accessing data could cause a runtime error.
| const { data } = await this.httpClient.request<Array<{ provider: number; code: string }>>(url, { timeout: 8000 }); | |
| const allowed = new Set((data || []).map((d) => d.code)); | |
| const response = await this.httpClient.request<Array<{ provider: number; code: string }>>(url, { timeout: 8000 }); | |
| const allowed = new Set((response?.data || []).map((d) => d.code)); |
| try { | ||
| const globals = await this.fetchGlobalEmoteCodes(); | ||
| for (const code of globals) allowed.add(code); | ||
| } catch {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Possible Bug
The empty catch block on line 194 silently ignores any errors that occur while fetching global emotes. This could hide important errors and lead to incomplete emote filtering since the global emotes won't be added to the allowed set.
| try { | |
| const globals = await this.fetchGlobalEmoteCodes(); | |
| for (const code of globals) allowed.add(code); | |
| } catch {} | |
| try { | |
| const globals = await this.fetchGlobalEmoteCodes(); | |
| for (const code of globals) allowed.add(code); | |
| } catch (error) { | |
| console.error('Failed to fetch global emotes:', error); | |
| } |
| const { data } = await this.httpClient.request<Array<{ provider: number; code: string }>>(url, { timeout: 8000 }); | ||
| for (const em of data || []) codes.add(em.code); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Possible Bug
The code doesn't properly validate the HTTP response data before using it. If data is null or undefined, data || [] will prevent an error, but it might indicate an API issue that should be handled explicitly.
| const { data } = await this.httpClient.request<Array<{ provider: number; code: string }>>(url, { timeout: 8000 }); | |
| for (const em of data || []) codes.add(em.code); | |
| const { data } = await this.httpClient.request<Array<{ provider: number; code: string }>>(url, { timeout: 8000 }); | |
| if (!data) { | |
| console.warn('No data received from global emotes API'); | |
| return codes; | |
| } | |
| for (const em of data) codes.add(em.code); |
| const current = (img as unknown as { currentSrc?: string }).currentSrc; | ||
| if (current) return current; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Possible Bug
The type assertion for currentSrc is unsafe and could lead to runtime errors. It's better to use optional chaining and proper type checking.
| const current = (img as unknown as { currentSrc?: string }).currentSrc; | |
| if (current) return current; | |
| const current = img.currentSrc; | |
| if (current) return current; |
| const first = srcset.split(",")[0]?.trim().split(" ")[0]; | ||
| if (first) { | ||
| return first.startsWith("//") ? `${window.location.protocol}${first}` : first; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Possible Bug
The current srcset parsing logic is unsafe and may return invalid URLs. The code assumes the first URL in srcset is valid and doesn't handle cases where the srcset format is invalid or empty after splitting. This could lead to broken image sources in the emote bar.
| const first = srcset.split(",")[0]?.trim().split(" ")[0]; | |
| if (first) { | |
| return first.startsWith("//") ? `${window.location.protocol}${first}` : first; | |
| } | |
| const parts = srcset.split(',')[0]?.trim().split(/\s+/); | |
| if (parts?.[0]) { | |
| const url = parts[0]; | |
| return url.startsWith('//') ? `${window.location.protocol}${url}` : url; | |
| } |
| const storage = (await this.localStorage().get("emoteBarByChannel")) || ({} as Record<string, EmoteItem[]>); | ||
| const key = this.getChannelKey(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Possible Bug
The getChannelKey() is called after loading storage data. If the channel changes between these operations, it could lead to loading and filtering emotes for the wrong channel.
| const storage = (await this.localStorage().get("emoteBarByChannel")) || ({} as Record<string, EmoteItem[]>); | |
| const key = this.getChannelKey(); | |
| const key = this.getChannelKey(); | |
| const storage = (await this.localStorage().get("emoteBarByChannel")) || ({} as Record<string, EmoteItem[]>); |
| try { | ||
| filtered = await this.filterEmotesAgainstChannel(loaded); | ||
| } catch {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Possible Bug
The empty catch block silently swallows any errors from filterEmotesAgainstChannel. This could hide critical issues like network failures or invalid data. Errors should be logged or handled appropriately.
| try { | |
| filtered = await this.filterEmotesAgainstChannel(loaded); | |
| } catch {} | |
| try { | |
| filtered = await this.filterEmotesAgainstChannel(loaded); | |
| } catch (error) { | |
| console.error('Failed to filter emotes:', error); | |
| filtered = loaded; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Key Issues
The error message in the catch block is misleading as it only mentions 'global emotes' while the try block handles both global and channel emotes, potentially obscuring channel emote fetch failures and complicating debugging.
| } catch (error) { | ||
| this.logger.warn("Failed to fetch global emotes:", error); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Possible Bug
The error message in the catch block only mentions 'global emotes' but the try block fetches both global and channel emotes. This could hide channel emote fetch failures and make debugging more difficult.
| } catch (error) { | |
| this.logger.warn("Failed to fetch global emotes:", error); | |
| } | |
| } catch (error) { | |
| this.logger.warn("Failed to fetch emotes:", error); | |
| } |
| @@ -1,3 +1,4 @@ | |||
| export type TwitchStorage = { | |||
| pinnedStreamers: string[]; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pinnedStreamers do wywalenia
| @@ -1,3 +1,4 @@ | |||
| export type TwitchStorage = { | |||
| pinnedStreamers: string[]; | |||
| emoteBarByChannel?: Record<string, { src: string; alt: string }[]>; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dodać tutaj typ
|
|
||
| private async fetchChannelEmotes(username: string): Promise<Set<string>> { | ||
| const codes = new Set<string>(); | ||
| const url = `${this.BACKEND_URL}/v1/channel/${encodeURIComponent(username)}/emotes/all`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wszystkie emotki powinny być ogólne, czyli zebyśmy mogli korzystać z nich w innych modułach, nie ma sensu pobierania ich per moduł
wrzuciłbym pobieranie emotek do modułu chatu, dokładnie tam gdzie pobieramy badge z naszego api w odpowieniej klasie
src/shared/apis/enhancer.api.ts
Outdated
| const url = `${EnhancerApi.EMOTES_API_URL}/v1/channel/${encodeURIComponent(username)}/emotes/all`; | ||
|
|
||
| try { | ||
| const response = await this.httpClient.request<Array<{ provider: number; code: string }>>(url, { timeout: 8000 }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add a type here
| export default class EnhancerApi { | ||
| private currentChannelId = ""; | ||
| private readonly cache = new Map<string, any>(); | ||
| private readonly channelEmotesCache = new Map<string, Set<string>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets use a Queue here, so we can set an expire for objects and avoid memory leaks
| import { render } from "preact"; | ||
| import styled from "styled-components"; | ||
|
|
||
| type EmoteItem = { src: string; alt: string; isWide?: boolean }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to types folder
* feat: add experimental tab
Bumps the npm_and_yarn group with 1 update in the / directory: [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite). Updates `vite` from 6.3.5 to 7.1.11 - [Release notes](https://github.com/vitejs/vite/releases) - [Changelog](https://github.com/vitejs/vite/blob/main/packages/vite/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite/commits/v7.1.11/packages/vite) --- updated-dependencies: - dependency-name: vite dependency-version: 7.1.11 dependency-type: direct:development dependency-group: npm_and_yarn ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
cd5a83c to
27dea95
Compare
Description
Briefly explain what this PR does. Is it a bug fix, new feature, or a refactor?
Testing
Select all the environments you tested this PR with:
Twitch
Kick
Please describe how you tested this change in the selected environments.
Related Issues
If this PR addresses an issue, link it here (e.g.,
Closes #123).✨
Description by Callstackai
This PR introduces a new Emote Bar feature for Twitch, allowing users to see and use emotes in chat. It includes the implementation of the EmoteBarModule, updates to settings, and necessary utility functions.
Diagrams of code changes
sequenceDiagram participant User participant EmoteBar participant EmoteBarModule participant TwitchChat participant EmotesAPI User->>TwitchChat: Sends chat message with emotes TwitchChat->>EmoteBarModule: Triggers handleMessage event EmoteBarModule->>EmoteBarModule: Extract emotes from message EmoteBarModule->>EmotesAPI: Fetch global emotes EmotesAPI-->>EmoteBarModule: Return global emote codes EmoteBarModule->>EmotesAPI: Fetch channel emotes EmotesAPI-->>EmoteBarModule: Return channel emote codes EmoteBarModule->>EmoteBar: Update emote bar display User->>EmoteBar: Click emote (normal click) EmoteBar->>TwitchChat: Insert emote in chat input User->>EmoteBar: Click emote (Ctrl+click) EmoteBar->>TwitchChat: Send emote directly to chatFiles Changed