build: use an npm font package instead of bundling#238
Open
SlayerOrnstein wants to merge 8 commits intoWFCD:mainfrom
Open
build: use an npm font package instead of bundling#238SlayerOrnstein wants to merge 8 commits intoWFCD:mainfrom
SlayerOrnstein wants to merge 8 commits intoWFCD:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaced Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/generator.ts (1)
1-234:⚠️ Potential issue | 🟡 MinorFormatter mismatch is currently CI-blocking.
The pipeline reports formatting drift. Please run the formatter and commit the result before merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/generator.ts` around lines 1 - 234, The CI failure is due to formatting drift in src/generator.ts (affecting functions generateCollapsed and generate); run the project's formatter (e.g., the repo's configured prettier/format script such as npm run format or yarn format), apply the formatted changes to this file and any others the formatter updates, then commit those changes so the file (including generateCollapsed and generate) matches the repository formatting rules and clears the CI check.src/utils.ts (1)
123-136:⚠️ Potential issue | 🟡 Minor
wrapTextcan emit empty first lines and mis-measure the first token.The current concatenation always prefixes a space, and when the first word exceeds
maxWidth, it pushes an empty line.💡 Proposed fix
- words.forEach((word) => { - const line = `${currentLine} ${word}`; + words.forEach((word) => { + const line = currentLine ? `${currentLine} ${word}` : word; const lineWidth = context.measureText(line).width; - if (lineWidth > maxWidth) { + if (lineWidth > maxWidth && currentLine) { lines.push(currentLine); currentLine = word; } else { currentLine = line; } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.ts` around lines 123 - 136, In wrapText, avoid always prefixing a space and handle the case where the first token itself exceeds maxWidth: build the candidate string as (currentLine ? `${currentLine} ${word}` : word) before measuring, and when lineWidth > maxWidth push currentLine only if it is non-empty; if currentLine is empty push the word (or start a new line with the word) and reset currentLine appropriately; reference variables/functions: wrapText, words, currentLine, line, lineWidth, maxWidth, context.measureText.
🧹 Nitpick comments (1)
src/utils.ts (1)
143-148: Clean up dead commented code inregisterFonts.Leaving the old implementation commented out makes font-loading behavior unclear. Prefer a concise no-op with an explicit comment (or a real implementation).
♻️ Suggested cleanup
export const registerFonts = () => { - // const fontPath = join('.', 'assets', 'fonts'); - // GlobalFonts.registerFromPath(join(fontPath, 'Roboto-Light.ttf'), 'Roboto'); - // GlobalFonts.registerFromPath(join(fontPath, 'Roboto-Regular.ttf'), 'Roboto'); - // GlobalFonts.registerFromPath(join(fontPath, 'Roboto-Bold.ttf'), 'Roboto'); + // Intentionally blank: font setup handled by runtime/package integration. };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.ts` around lines 143 - 148, Remove the dead commented-out font-loading code inside the registerFonts function and replace it with a clear no-op comment or a real implementation; locate the registerFonts function and either (a) leave it as an explicit no-op with a short comment like "no fonts to register in this environment" or (b) implement actual font registration using GlobalFonts.registerFromPath and join if you intend to enable fonts, making sure to reference GlobalFonts and join where appropriate so the function's behavior is unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/generator.ts`:
- Line 4: The bare import of "@fontsource-variable/roboto" should be removed and
real canvas font registration must be implemented in registerFonts(): obtain the
actual font file(s) (.ttf/.woff2/.otf), call GlobalFonts.registerFromPath(path,
"Roboto") or GlobalFonts.register(buffer, "Roboto") from `@napi-rs/canvas` before
any canvas creation (reference the registerFonts() function), and make sure the
family name "Roboto" matches the family used in ctx.font where text is drawn;
alternatively remove the registerFonts() call entirely if you choose to rely on
system fonts.
---
Outside diff comments:
In `@src/generator.ts`:
- Around line 1-234: The CI failure is due to formatting drift in
src/generator.ts (affecting functions generateCollapsed and generate); run the
project's formatter (e.g., the repo's configured prettier/format script such as
npm run format or yarn format), apply the formatted changes to this file and any
others the formatter updates, then commit those changes so the file (including
generateCollapsed and generate) matches the repository formatting rules and
clears the CI check.
In `@src/utils.ts`:
- Around line 123-136: In wrapText, avoid always prefixing a space and handle
the case where the first token itself exceeds maxWidth: build the candidate
string as (currentLine ? `${currentLine} ${word}` : word) before measuring, and
when lineWidth > maxWidth push currentLine only if it is non-empty; if
currentLine is empty push the word (or start a new line with the word) and reset
currentLine appropriately; reference variables/functions: wrapText, words,
currentLine, line, lineWidth, maxWidth, context.measureText.
---
Nitpick comments:
In `@src/utils.ts`:
- Around line 143-148: Remove the dead commented-out font-loading code inside
the registerFonts function and replace it with a clear no-op comment or a real
implementation; locate the registerFonts function and either (a) leave it as an
explicit no-op with a short comment like "no fonts to register in this
environment" or (b) implement actual font registration using
GlobalFonts.registerFromPath and join if you intend to enable fonts, making sure
to reference GlobalFonts and join where appropriate so the function's behavior
is unambiguous.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.jsonsrc/drawers.tssrc/generator.tssrc/utils.tstests/generator.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package.json (1)
19-19: Minor: Spec pattern inconsistency with Mocha config.The test command uses
tests/*.spec.ts(non-recursive), while.mocharc.ymlspecifiestests/**/*.spec.ts(recursive). If Mocha reads from.mocharc.yml, the command-line pattern may be overridden, but it's cleaner to keep them consistent.Suggested fix
- "test": "c8 mocha tests/*.spec.ts", + "test": "c8 mocha",Since
.mocharc.ymlalready defines the spec pattern, the command-line argument can be omitted to avoid duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 19, The package.json "test" script currently specifies a non-recursive pattern ("tests/*.spec.ts") that conflicts with the recursive pattern in .mocharc.yml; remove the explicit pattern from the "test" script so it simply runs the test runner (e.g., change the value of the "test" script to "c8 mocha" or equivalent) and rely on .mocharc.yml's tests/**/*.spec.ts pattern to keep patterns consistent and avoid duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@package.json`:
- Line 19: The package.json "test" script currently specifies a non-recursive
pattern ("tests/*.spec.ts") that conflicts with the recursive pattern in
.mocharc.yml; remove the explicit pattern from the "test" script so it simply
runs the test runner (e.g., change the value of the "test" script to "c8 mocha"
or equivalent) and rely on .mocharc.yml's tests/**/*.spec.ts pattern to keep
patterns consistent and avoid duplication.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.mocharc.ymlbiome.jsonlint-staged.config.jspackage.jsonsrc/generator.tssrc/utils.tstsdown.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/generator.ts
- src/utils.ts
TobiTenno
requested changes
Feb 26, 2026
Co-authored-by: Matt <7128721+TobiTenno@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.babelrc.json:
- Around line 3-7: The .babelrc.json references three Babel packages that are
not declared in package.json; add `@babel/preset-env`,
`@babel/plugin-transform-class-properties`, and
`@babel/plugin-transform-private-methods` to package.json's devDependencies so
installs/CI succeed; update the devDependencies section to include those exact
package names (with appropriate versions or use a caret range) and run npm/yarn
install to verify resolution.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.babelrc.json.c8rc.jsonpackage.json
✅ Files skipped from review due to trivial changes (1)
- .c8rc.json
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
Don't think its being used transitive and I'm pretty sure tsdown already does some of this
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What did you fix?
Pretty much the title, import fonts from a package and move
warframe-itemsto the@wfcdnamespaceReproduction steps
Evidence/screenshot/link to line
Considerations
Summary by CodeRabbit
New Features
Dependencies
Testing
Chores