update font loader #1883
Closed
update font loader #1883
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 5c09f48 in 2 minutes and 31 seconds. Click for details.
- Reviewed
305lines of code in5files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/font-panel/font-family.tsx:49
- Draft comment:
Using a fixed '12px' in document.fonts.check may not account for weight or style variations. Consider parameterizing the font size. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The font size in document.fonts.check() is just used to test if the font family is loaded - it's not related to the actual display size. The MDN docs don't indicate that font size affects the check. The weight and style are handled separately in the component's rendering. The font size here is just a required parameter for the check API. I could be wrong about how document.fonts.check() works internally - maybe different sizes do affect detection of certain font variations. Even if font size could theoretically matter, there's no clear evidence that it would improve anything here. The code already handles weights and styles appropriately in the actual rendering. The comment should be deleted as it suggests a change without clear evidence that it would improve anything. The current implementation appears to work correctly for its purpose.
2. apps/web/client/src/app/project/[id]/_components/left-panel/brand-tab/font-panel/font-family.tsx:45
- Draft comment:
Ensure local font 'src' values are trusted or sanitized, as they are directly used by ensureLocalFontLoaded. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While security is important, this seems like a speculative concern. The src values come from the component's props which would be controlled by the application, not direct user input. Without seeing how localSrcs is populated, we can't definitively say there's a security issue. The comment is also phrased as a verification request ("Ensure that...") which violates our rules. I could be wrong about the source of the font data - perhaps there is user input involved earlier in the flow that I can't see. Security concerns shouldn't be dismissed lightly. While security is important, we need clear evidence of an issue to keep comments. This comment is speculative and asks for verification rather than pointing out a concrete problem. Delete this comment because it's speculative, phrased as a verification request, and lacks clear evidence of an actual security issue in the changed code.
3. apps/web/client/src/components/store/editor/font/index.ts:202
- Draft comment:
Removal of WebFont loader looks intentional; ensure that Next.js font optimization covers all use cases, especially for older browsers. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%The comment suggests ensuring that Next.js font optimization covers all use cases, which is a form of asking the author to double-check something. This violates the rule against asking the author to ensure behavior is intended or tested. However, it does point out a specific concern about older browsers, which could be useful if rephrased to focus on confirming the intention of removing WebFont loader.
4. apps/web/client/src/utils/localFontPreview.ts:1
- Draft comment:
Ensure that the 'src' URL is trusted since it is injected into a style tag's innerHTML. Consider sanitizing inputs to avoid potential XSS risks. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This is a legitimate security concern - innerHTML injection with unsanitized input can lead to XSS attacks. However, this is a font loading utility where src is expected to be a font file URL and fontFamily a font name. The risk is relatively low since font-face src only accepts valid font URLs and font-family values are CSS-escaped. The comment doesn't provide specific actionable guidance on what sanitization to perform. I might be underestimating the security risk. Even if the expected inputs are benign, malicious input could potentially exploit this. While there is a theoretical security risk, the context suggests this is an internal font preview utility where inputs are likely controlled. The browser's CSS parser also provides some natural sanitization. The comment raises a valid security concern but isn't actionable enough and may be overly cautious given the context. Better to delete it unless more specific guidance can be provided.
Workflow ID: wflow_bWhYqhWCgtKrFjzu
You can customize 
| @@ -15,10 +15,35 @@ const FontPanel = observer(() => { | |||
| const [isLoading, setIsLoading] = useState(false); | |||
| const inputRef = useRef<HTMLInputElement>(null); | |||
| const [isUploadModalOpen, setIsUploadModalOpen] = useState(false); | |||
| const [loadedFonts, setLoadedFonts] = useState<Set<string>>(new Set()); | |||
There was a problem hiding this comment.
The loadedFonts state is being computed but not used in rendering. Consider removing it if redundant.
| @@ -0,0 +1,13 @@ | |||
| export function ensureGoogleFontLoaded(fontFamily: string, weights: (string | number)[] = ['400']) { | |||
| if (!fontFamily) return; | |||
| const fontId = fontFamily.replace(/ /g, '+'); | |||
There was a problem hiding this comment.
Consider sanitizing or encoding the fontFamily input before constructing the Google Fonts URL to mitigate potential injection risks.
Suggested change
| const fontId = fontFamily.replace(/ /g, '+'); | |
| const fontId = encodeURIComponent(fontFamily.replace(/ /g, '+')); |
|
Close and revisit later |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Update font loading to use Next.js optimization and add utilities for Google and local font loading.
WebFontloader with Next.js font optimization inFontManager.ensureGoogleFontLoadedinfontPreview.tsandensureLocalFontLoadedinlocalFontPreview.tsto manage font loading.FontFamilyandFontVariantcomponents infont-family.tsxto use new font loading utilities.useEffecthooks to check and set font loading status.WebFontimport fromfont/index.ts.font-family.tsxto reflect font loading status.This description was created by
for 5c09f48. You can customize this summary. It will automatically update as commits are pushed.