-
Notifications
You must be signed in to change notification settings - Fork 21
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
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.
Pull request overview
This PR introduces a new Emote Bar feature for Twitch that displays recently used emotes in chat, allowing users to quickly reuse them. The implementation includes emote tracking, persistence per channel, filtering based on available emotes, and a new "Experimental" settings tab.
- Adds EmoteBarModule to track and display user's recently sent emotes
- Implements emote filtering API endpoints with caching
- Refactors settings module to use dynamic tab indexing with new "Experimental" tab
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/platforms/twitch/modules/emote-bar/emote-bar.module.tsx | New module implementing the emote bar feature with persistence and UI |
| src/shared/apis/enhancer.api.ts | Adds API methods to fetch global and channel emotes with caching |
| src/platforms/twitch/modules/settings/settings.module.tsx | Adds experimental tab and refactors tab indexing from hardcoded to dynamic |
| src/platforms/kick/modules/settings/settings.module.tsx | Applies same experimental tab and tab indexing refactor for consistency |
| src/types/platforms/twitch/twitch.storage.types.ts | Defines storage type for emote bar (removes pinnedStreamers) |
| src/types/platforms/twitch/twitch.utils.types.ts | Adds EmoteItem type with src, alt, and isWide properties |
| src/types/platforms/twitch/twitch.settings.types.ts | Adds emoteBarEnabled setting |
| src/types/apis/enhancer.apis.ts | Adds EnhancerEmote type for API responses |
| src/platforms/twitch/twitch.constants.ts | Sets emoteBarEnabled default to false |
| src/platforms/twitch/twitch.platform.ts | Registers EmoteBarModule in platform modules |
| src/platforms/twitch/twitch.utils.ts | Increases React traversal depth from 100 to 190 |
| public/assets/settings/experimental.svg | Adds icon for experimental settings tab |
| package.json | Updates version to 5.1.16 and vite to 7.1.11 |
| package-lock.json | Updates dependencies to match package.json changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,3 +1,8 @@ | |||
| export type TwitchEmoteBarItem = { | |||
| src: string; | |||
| alt: string; | |||
Copilot
AI
Dec 28, 2025
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.
The TwitchEmoteBarItem type is missing the isWide property that is present in EmoteItem. This could cause data loss when persisting emotes, as the isWide property is used for layout calculations but won't be stored. Consider adding the isWide?: boolean property to TwitchEmoteBarItem to maintain consistency.
| alt: string; | |
| alt: string; | |
| isWide?: boolean; |
| let filtered = loaded; | ||
| try { | ||
| filtered = await this.filterEmotesAgainstChannel(loaded); | ||
| } catch {} |
Copilot
AI
Dec 28, 2025
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.
Empty catch block silently swallows errors during emote filtering. Consider at least logging the error to help with debugging, especially since this affects the user experience when switching channels.
| } catch {} | |
| } catch (error) { | |
| this.logger.warn("Failed to filter persisted emotes for channel", key, error); | |
| } |
| // Note: Your 'EmoteItem' type no longer needs the 'isWide' property | ||
|
|
Copilot
AI
Dec 28, 2025
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.
The comment states "Your 'EmoteItem' type no longer needs the 'isWide' property" but the type still includes it and it's actively used in the code (lines 49, 88, 113, 237). This misleading comment should be removed or updated to reflect the actual implementation.
| // Note: Your 'EmoteItem' type no longer needs the 'isWide' property |
| gap: 8px 10px; | ||
|
|
||
| /* Still enforces the 2-row limit */ | ||
| height: 92px; /* 36(row) + 8(gap) + 36(row) + 6(pad) + 6(pad) */ |
Copilot
AI
Dec 28, 2025
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.
The height calculation comment "36(row) + 8(gap) + 36(row) + 6(pad) + 6(pad)" equals 92px, but the actual emote height is 28px (line 269), not 36px. This discrepancy suggests the height value may need adjustment or the comment needs correction.
| height: 92px; /* 36(row) + 8(gap) + 36(row) + 6(pad) + 6(pad) */ | |
| height: 92px; /* Enforces 2 rows: 2 × 28px emotes + 8px row gap + vertical padding/line spacing */ |
| const weight = item.isWide ? 2 : 1; | ||
| if (used + weight > MAX_SLOTS) break; | ||
|
|
||
| if (weight === 2 && column % 9 === 8) continue; |
Copilot
AI
Dec 28, 2025
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.
The value 9 appears to be a magic number representing the number of columns in the emote grid. Consider extracting this to a named constant like COLUMNS_PER_ROW = 9 to make the code more maintainable and the logic clearer.
| @@ -100,7 +100,7 @@ export default class TwitchUtils { | |||
| const node = this.reactUtils.findReactChildren<Chat>( | |||
| this.reactUtils.getReactInstance(document.querySelector(".stream-chat")), | |||
| (n) => n.stateNode?.props?.onSendMessage, | |||
Copilot
AI
Dec 28, 2025
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.
The search depth was increased from 100 to 190 without explanation. Consider adding a comment explaining why this specific depth is needed, especially since it nearly doubles the traversal depth which could impact performance.
| (n) => n.stateNode?.props?.onSendMessage, | |
| (n) => n.stateNode?.props?.onSendMessage, | |
| // NOTE: A relatively high depth is required here because the Chat component | |
| // is nested deeply in Twitch's React tree; lower values (e.g. 100) have | |
| // failed to find the node after Twitch layout updates. |
|
|
||
| export type TwitchStorage = { | ||
| pinnedStreamers: string[]; | ||
| emoteBarByChannel?: Record<string, TwitchEmoteBarItem[]>; |
Copilot
AI
Dec 28, 2025
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.
The pinnedStreamers field has been removed from TwitchStorage, but it is still defined in TwitchSettings and used throughout the codebase (e.g., in pin-streamer.module.tsx). This appears to be an unintended breaking change. The pinnedStreamers property should remain in the storage type, or the pinned streamers feature should be migrated to use settings storage consistently.
| emoteBarByChannel?: Record<string, TwitchEmoteBarItem[]>; | |
| emoteBarByChannel?: Record<string, TwitchEmoteBarItem[]>; | |
| pinnedStreamers?: string[]; |
| src={item.src} | ||
| alt={item.alt} | ||
| onClick={(e) => (e.ctrlKey ? onSend(item.alt) : onInsert(item.alt))} | ||
| /* The inline 'style' prop is gone! */ |
Copilot
AI
Dec 28, 2025
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.
The comment states "The inline 'style' prop is gone!" but this appears to be leftover from development. This comment should be removed as it doesn't provide meaningful documentation.
| /* The inline 'style' prop is gone! */ |
| private async filterEmotesAgainstChannel(emotes: EmoteItem[]): Promise<EmoteItem[]> { | ||
| if (emotes.length === 0) return emotes; | ||
|
|
||
| const info = this.twitchUtils().getChannelInfo() || this.twitchUtils().getChannelInfoFromHomeLowerContent(); | ||
| const username = info?.channelLogin || this.twitchUtils().getCurrentChannelByUrl(); | ||
| if (!username) return emotes; | ||
| const allowed = new Set(); | ||
| try { | ||
| const globals = await this.enhancerApi().getGlobalEmotes(); | ||
| const channel = await this.enhancerApi().getChannelEmotes(username); | ||
| for (const code of globals) allowed.add(code); | ||
| for (const code of channel) allowed.add(code); | ||
| } catch (error) { | ||
| this.logger.warn("Failed to fetch global emotes:", error); | ||
| } | ||
| return emotes.filter((e) => !!e.alt && allowed.has(e.alt)); | ||
| } |
Copilot
AI
Dec 28, 2025
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.
The filterEmotesAgainstChannel method fetches both global and channel emotes on every channel load. Consider caching the emote sets in the module to avoid redundant API calls when switching between channels, as global emotes don't change per channel.
| try { | ||
| const response = await this.httpClient.request<EnhancerEmote[]>(url, { timeout: 8000 }); | ||
| const data = response.data || []; | ||
| for (const em of data || []) codes.add(em.code); | ||
| this.globalEmotesCache = codes; | ||
| } catch (error) { | ||
| this.logger.warn("Failed to fetch global emotes:", error); |
Copilot
AI
Dec 28, 2025
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.
The error message "Failed to fetch global emotes:" is used in line 186 of emote-bar.module.tsx even when channel emotes fail to fetch. The message should be more generic or the error should distinguish between global and channel emote fetch failures.
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