perf: prevent duplicates in file tree sprite#2586
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThe Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/generate-file-tree-sprite.ts (2)
57-57: Consider deduplicatingiconNamesonce at source.Deduplicating inside
buildSpriteworks, butgroupByCollectionstill iterates and pushes duplicates into each group, wasting work and memory. Dedup once at the call site keepsbuildSprite/groupByCollectionsimpler and equally correct.♻️ Proposed refactor
- new Set(iconNames).forEach(name => { + iconNames.forEach(name => {-const iconNames = [ +const iconNames = [...new Set([ ...Object.values(EXTENSION_ICONS), ...Object.values(FILENAME_ICONS), ...Object.values(COMPOUND_EXTENSIONS), ...Object.values(ADDITIONAL_ICONS), DEFAULT_ICON, -] +])]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-file-tree-sprite.ts` at line 57, Deduplicate iconNames before it's passed into buildSprite/groupByCollection by converting it to a unique list (e.g., create a Set from iconNames and use that array for subsequent calls) so groupByCollection no longer receives duplicates; then remove the in-buildSprite deduplication (the new Set(iconNames).forEach(...) usage) or any redundant filtering inside groupByCollection to keep them simpler and avoid double work—update all call sites that pass iconNames to use the deduplicated array and adjust buildSprite and groupByCollection to assume input is already unique.
72-91: Loss of explicit error handling on top-level await.Dropping the
main().catch(err => { console.error(err); process.exit(1) })wrapper means error propagation now relies on Node's default unhandled-rejection behaviour to produce a non-zero exit code. Since this script is invoked viaexecSyncfromvite.config.ts, a missing/zero exit code on failure would silently pass a broken build. Node ≥15 defaults to--unhandled-rejections=throw(exit 1), so current behaviour is likely preserved, but:
- Stack traces from top-level await rejections are less readable than the explicit
console.error(error)output.- Other scripts in this repo (
scripts/generate-fixtures.ts,scripts/release-notes.ts) still use the explicitmain().catchpattern — this diverges from that convention.Consider restoring an explicit handler, or wrapping in a try/catch, for consistency and clearer failure output:
🛡️ Suggested pattern
-const collections = await loadCollections() -const iconNames = [ +try { + const collections = await loadCollections() + const iconNames = [ ... -await Promise.all([ - fs.writeFile(outputDevPath, sprite, 'utf8'), - fs.writeFile(outputStagePath, sprite, 'utf8'), - fs.writeFile(outputProdPath, sprite, 'utf8'), -]) + await Promise.all([ + fs.writeFile(outputDevPath, sprite, 'utf8'), + fs.writeFile(outputStagePath, sprite, 'utf8'), + fs.writeFile(outputProdPath, sprite, 'utf8'), + ]) +} catch (error) { + console.error(error) + process.exit(1) +}As per coding guidelines: "Use error handling patterns consistently".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-file-tree-sprite.ts` around lines 72 - 91, Wrap the top-level async flow in an explicit error handler for consistent, readable failures: invoke main() (the async function that performs loadCollections, groupByCollection and buildSprite and writes outputDevPath/outputStagePath/outputProdPath) with .catch(err => { console.error(err); process.exit(1); }) or, if you prefer a single-file pattern, surround the body in try { ... } catch (err) { console.error(err); process.exit(1); } so any rejection from loadCollections, buildSprite, fs.mkdir, or fs.writeFile yields a clear log and non-zero exit code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/generate-file-tree-sprite.ts`:
- Line 57: Deduplicate iconNames before it's passed into
buildSprite/groupByCollection by converting it to a unique list (e.g., create a
Set from iconNames and use that array for subsequent calls) so groupByCollection
no longer receives duplicates; then remove the in-buildSprite deduplication (the
new Set(iconNames).forEach(...) usage) or any redundant filtering inside
groupByCollection to keep them simpler and avoid double work—update all call
sites that pass iconNames to use the deduplicated array and adjust buildSprite
and groupByCollection to assume input is already unique.
- Around line 72-91: Wrap the top-level async flow in an explicit error handler
for consistent, readable failures: invoke main() (the async function that
performs loadCollections, groupByCollection and buildSprite and writes
outputDevPath/outputStagePath/outputProdPath) with .catch(err => {
console.error(err); process.exit(1); }) or, if you prefer a single-file pattern,
surround the body in try { ... } catch (err) { console.error(err);
process.exit(1); } so any rejection from loadCollections, buildSprite, fs.mkdir,
or fs.writeFile yields a clear log and non-zero exit code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 14b74b12-5ba6-4193-8439-1f70d600775e
📒 Files selected for processing (1)
scripts/generate-file-tree-sprite.ts
serhalp
left a comment
serhalp
left a comment
There was a problem hiding this comment.
LGTM, good find
It would be nice to add some perf benchmarks to CI eventually!
2 participants
🔗 Linked issue
n/a
🧭 Context
#1347
📚 Description
I noticed that there is duplicate icons in the file tree sprite, which doesn't seem to be intended - but I may be wrong. If I'm correct then this would shrink it currently from 576K to 260K 🎉