Conversation
Summary of ChangesHello @igorovh, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces dynamic font loading capabilities, allowing the application to fetch and apply custom fonts based on user settings in both global and current channel contexts. It refines the font loading mechanism to efficiently manage font stylesheets in the DOM and enhances the channel joining API to provide clearer feedback and emit a dedicated event upon successful channel entry. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the additional fonts module to load fonts dynamically based on user settings, which is a great enhancement. The changes are generally good, but I've provided feedback on improving the font collection logic for better performance and readability. I've also pointed out a potential issue with long font lists creating URLs that exceed browser limits and suggested a truncation mechanism to make the feature more robust. This truncation logic is needed in two places, and I've suggested extracting it to a helper method to avoid duplication.
|
|
||
| private async run(elements: Element[]) { | ||
| this.additionalFontsHelper.loadFonts(elements, AdditionalFontsModule.FONT_LIST); | ||
| this.additionalFontsHelper.loadFonts(elements, this.getUsedFonts()); |
There was a problem hiding this comment.
The list of fonts from getUsedFonts() can potentially be very long, which might create a URL that exceeds the browser's maximum length limit when calling the Google Fonts API. This could cause font loading to fail. It's advisable to truncate the font list to a reasonable maximum size before passing it to loadFonts.
| this.additionalFontsHelper.loadFonts(elements, this.getUsedFonts()); | |
| let fonts = this.getUsedFonts(); | |
| const MAX_FONTS = 50; | |
| if (fonts.length > MAX_FONTS) { | |
| this.logger.warn(`Too many fonts to load (${fonts.length}), truncating to ${MAX_FONTS}`); | |
| fonts = fonts.slice(0, MAX_FONTS); | |
| } | |
| this.additionalFontsHelper.loadFonts(elements, fonts); |
| public async updateFonts() { | ||
| const head = document.querySelector("head"); | ||
| if (!head) return; | ||
| this.additionalFontsHelper.loadFonts([head], this.getUsedFonts()); |
There was a problem hiding this comment.
Similar to my other comment, the list of fonts should be truncated here as well to avoid creating an overly long URL. To avoid code duplication between this method and run, you could extract this font loading logic into a private helper method.
let fonts = this.getUsedFonts();
const MAX_FONTS = 50;
if (fonts.length > MAX_FONTS) {
this.logger.warn(`Too many fonts to load (${fonts.length}), truncating to ${MAX_FONTS}`);
fonts = fonts.slice(0, MAX_FONTS);
}
this.additionalFontsHelper.loadFonts([head], fonts);
| getUsedFonts(): string[] { | ||
| const globalFonts = | ||
| this.enhancerApi() | ||
| .getGlobalChannel() | ||
| ?.users.map((user) => user.customFont) ?? []; | ||
| const currentFonts = | ||
| this.enhancerApi() | ||
| .getCurrentChannel() | ||
| ?.users.map((user) => user.customFont) ?? []; | ||
|
|
||
| const allRawFonts = [...globalFonts, ...currentFonts]; | ||
|
|
||
| const uniqueFonts: string[] = []; | ||
| const seen = new Set<string>(); | ||
|
|
||
| for (const font of allRawFonts) { | ||
| if (!font) continue; | ||
|
|
||
| const normalized = font.toLowerCase().trim(); | ||
|
|
||
| if (!seen.has(normalized)) { | ||
| seen.add(normalized); | ||
| uniqueFonts.push(font); | ||
| } | ||
| } | ||
|
|
||
| return uniqueFonts; | ||
| } |
There was a problem hiding this comment.
This method can be simplified and made more efficient. You can combine the user arrays from global and current channels first, and then iterate once to build a map of unique fonts. This avoids multiple iterations and intermediate arrays.
getUsedFonts(): string[] {
const allUsers = [
...(this.enhancerApi().getGlobalChannel()?.users ?? []),
...(this.enhancerApi().getCurrentChannel()?.users ?? []),
];
const uniqueFonts = new Map<string, string>();
for (const user of allUsers) {
const font = user.customFont;
if (!font) {
continue;
}
const normalized = font.toLowerCase().trim();
if (!uniqueFonts.has(normalized)) {
uniqueFonts.set(normalized, font);
}
}
return Array.from(uniqueFonts.values());
}
There was a problem hiding this comment.
Pull request overview
This pull request implements dynamic font loading for user custom fonts. However, the title contains a spelling error ("addational" should be "additional"). The PR adds an event system to trigger font reloading when users join a channel, but the implementation is incomplete.
Changes:
- Added "extension:joined-channel" event type to common events
- Modified EnhancerApi.joinChannel to return boolean indicating success
- Updated font loading helper to update existing font links instead of always appending new ones
- Replaced static font list in Kick's AdditionalFontsModule with dynamic font retrieval from user data
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/platforms/common.events.ts | Added new event type for channel join notifications |
| src/shared/module/helpers/additional-fonts.helper.ts | Modified to update existing font links and added fontList length check |
| src/shared/apis/enhancer.api.ts | Changed return type from void to boolean to indicate join success |
| src/platforms/twitch/modules/chat/chat.module.tsx | Added event emission when successfully joining a channel |
| src/platforms/kick/modules/additional-fonts/additional-fonts.module.tsx | Replaced static font list with dynamic retrieval and added updateFonts method |
Comments suppressed due to low confidence (1)
src/shared/apis/enhancer.api.ts:65
- The method's return type is Promise, but when an error occurs in the try block, the method throws instead of returning false. This creates an inconsistent API where callers need to handle both the boolean return value and potential exceptions. Consider either always returning false on error (and logging it) or updating the calling code to properly handle the exception case.
} catch (error) {
this.logger.error(`Failed to join channel ${channelId}:`, error);
throw error;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
|
|
||
| private async run(elements: Element[]) { | ||
| this.additionalFontsHelper.loadFonts(elements, AdditionalFontsModule.FONT_LIST); | ||
| this.additionalFontsHelper.loadFonts(elements, this.additionalFontsHelper.getUsedFonts()); | ||
| } |
There was a problem hiding this comment.
The updateFonts method is defined but never called. To enable dynamic font loading when a channel is joined, this module should register an event listener for the "extension:joined-channel" event. Consider adding an event applier to the config that calls updateFonts when the event is emitted, similar to how the chat module listens to "twitch:chatInitialized".
|
|
||
| for (const head of elements) { | ||
| head.appendChild(fontLink.cloneNode(true)); | ||
| const existingLink = head.querySelector("link.enhancer-additional-font") as HTMLLinkElement; | ||
| if (existingLink) { | ||
| existingLink.href = url; | ||
| } else { |
There was a problem hiding this comment.
When updating an existing link's href, this creates a potential race condition. If the previous font is still loading when the href is changed, it could lead to incomplete font loading or visual inconsistencies. Consider removing the old link and appending a new one instead, or ensure that the previous load is complete before updating the href.
| for (const head of elements) { | |
| head.appendChild(fontLink.cloneNode(true)); | |
| const existingLink = head.querySelector("link.enhancer-additional-font") as HTMLLinkElement; | |
| if (existingLink) { | |
| existingLink.href = url; | |
| } else { | |
| head.removeChild(existingLink); | |
| } | |
| head.appendChild(newFontLink.cloneNode(true)); | |
| } | |
| } | |
| } |
| newFontLink.setAttribute("onload", "this.media='all'"); | ||
| newFontLink.classList.add("enhancer-additional-font"); | ||
|
|
||
| for (const head of elements) { | ||
| head.appendChild(fontLink.cloneNode(true)); | ||
| const existingLink = head.querySelector("link.enhancer-additional-font") as HTMLLinkElement; | ||
| if (existingLink) { |
There was a problem hiding this comment.
When there are multiple head elements and some have existing font links while others don't, the logic becomes inconsistent. If the first head element has an existing link, it will be updated with the new URL, but then the loop continues and subsequent heads without existing links will append new cloned links. This creates an inconsistent state where some heads have updated links and others have newly appended links. Consider handling all elements uniformly.
| newFontLink.setAttribute("onload", "this.media='all'"); | |
| newFontLink.classList.add("enhancer-additional-font"); | |
| for (const head of elements) { | |
| head.appendChild(fontLink.cloneNode(true)); | |
| const existingLink = head.querySelector("link.enhancer-additional-font") as HTMLLinkElement; | |
| if (existingLink) { | |
| const existingLinks = head.querySelectorAll("link.enhancer-additional-font"); | |
| existingLinks.forEach((link) => link.remove()); | |
| head.appendChild(newFontLink.cloneNode(true)); |
| export class AdditionalFontsHelper { | ||
| constructor(private readonly enhancerApi: EnhancerApi) {} |
There was a problem hiding this comment.
While adding the fontList.length check is good defensive programming, when an empty font list is passed, any existing font link elements with the "enhancer-additional-font" class will remain in the DOM. Consider whether this is the intended behavior, or if existing font links should be removed when there are no fonts to load.
| export class AdditionalFontsHelper { | |
| constructor(private readonly enhancerApi: EnhancerApi) {} | |
| if (elements.length === 0) return; | |
| if (fontList.length === 0) { | |
| for (const head of elements) { | |
| const existingLinks = head.querySelectorAll("link.enhancer-additional-font"); | |
| existingLinks.forEach((link) => { | |
| if (link.parentNode) { | |
| link.parentNode.removeChild(link); | |
| } | |
| }); | |
| } | |
| return; | |
| } |
|
|
||
| const familyQuery = fontList.map((font) => `family=${font.replace(/\s+/g, "+")}`).join("&"); | ||
| const url = `https://fonts.googleapis.com/css2?${familyQuery}&display=swap`; | ||
| const fontLink = document.createElement("link"); | ||
| fontLink.rel = "stylesheet"; | ||
| fontLink.href = url; | ||
|
|
||
| const newFontLink = document.createElement("link"); | ||
| newFontLink.rel = "stylesheet"; | ||
| newFontLink.href = url; |
There was a problem hiding this comment.
The variable is renamed from fontLink to newFontLink, but this name is slightly misleading since the link may not always be "new" - it's only used when creating a new link element, while existing links are updated in-place. Consider a more neutral name like fontLinkTemplate or fontLinkElement.
| const joined = await this.enhancerApi().joinChannel(channelId); | ||
| if (joined) { | ||
| this.emitter.emit("extension:joined-channel"); | ||
| this.logger.info(`Joined channel ${channelId}`); | ||
| } |
There was a problem hiding this comment.
The Kick chat module's initializeChannel method hasn't been updated to handle the new boolean return value from joinChannel and emit the "extension:joined-channel" event. This creates an inconsistency where the event is only emitted on Twitch but not on Kick, preventing the dynamic font loading feature from working properly on Kick. The Kick implementation should be updated to match the Twitch implementation pattern.
1 participant
No description provided.