Skip to content

feat: add binary file warning#1959

Open
ShroXd wants to merge 6 commits intonpmx-dev:mainfrom
ShroXd:binary-file-placeholder
Open

feat: add binary file warning#1959
ShroXd wants to merge 6 commits intonpmx-dev:mainfrom
ShroXd:binary-file-placeholder

Conversation

@ShroXd
Copy link
Contributor

@ShroXd ShroXd commented Mar 6, 2026

🔗 Linked issue

Resolves #1945

🧭 Context

The binary file is rendered directly, which is not a good user experience. Before supporting different binary file rendering (e.g. image), it would be better to show a warning and allow user to render it anyway.

📚 Description

Scenario Screenshot
Light mode Screenshot From 2026-03-08 09-34-49
Dark mode Screenshot From 2026-03-08 09-35-16

@vercel
Copy link

vercel bot commented Mar 6, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
docs.npmx.dev Ready Ready Preview, Comment Mar 9, 2026 1:07pm
npmx.dev Ready Ready Preview, Comment Mar 9, 2026 1:07pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
npmx-lunaria Ignored Ignored Mar 9, 2026 1:07pm

Request Review

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Lunaria Status Overview

🌕 This pull request will trigger status changes.

Learn more

By default, every PR changing files present in the Lunaria configuration's files property will be considered and trigger status changes accordingly.

You can change this by adding one of the keywords present in the ignoreKeywords property in your Lunaria configuration file in the PR's title (ignoring all files) or by including a tracker directive in the merged commit's description.

Tracked Files

File Note
i18n/locales/en.json Source changed, localizations will be marked as outdated.
Warnings reference
Icon Description
🔄️ The source for this localization has been updated since the creation of this pull request, make sure all changes in the source have been applied.

@codecov
Copy link

codecov bot commented Mar 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@ShroXd ShroXd marked this pull request as ready for review March 6, 2026 11:17
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 6, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c71006c0-54ea-4578-88c5-8fc2304a9076

📥 Commits

Reviewing files that changed from the base of the PR and between d81687a and 0a27556.

📒 Files selected for processing (2)
  • i18n/locales/en.json
  • i18n/schema.json
🚧 Files skipped from review as they are similar to previous changes (2)
  • i18n/schema.json
  • i18n/locales/en.json

📝 Walkthrough

Walkthrough

Adds binary-file detection and handling to the package code page. New utility app/utils/file-types.ts exports isBinaryFilePath(filePath: string): boolean backed by an internal BINARY_EXTENSIONS set. The page component computes isBinaryFile from the current path and uses it to short-circuit content fetching (returns null for binary paths) and to skip the standard file viewer. The UI now renders a binary-file warning block (including a CDN "view raw" link) and updates OG image/preview handling. Adds i18n keys common.binary_file and common.binary_rendering_warning.

Suggested reviewers

  • danielroe
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The PR description clearly relates to the changeset, explaining the context of handling binary files and showing UI screenshots.
Linked Issues check ✅ Passed The PR successfully implements the core requirement from #1945: preventing binary files from displaying as raw text and showing a clear placeholder warning instead.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing binary file handling: utility function for detection, UI warning component, and i18n translations. No unrelated modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue (1)

547-552: Consider adding target="_blank" and rel="noopener noreferrer" for the external link.

The "View raw file" link points to an external CDN URL but doesn't include the target="_blank" attribute. This differs from the similar raw file link on line 507-515, which opens in a new tab. For consistency and better UX (users likely expect external links to open in a new tab), consider adding these attributes.

Proposed fix
             <LinkBase
               variant="button-secondary"
               :to="`https://cdn.jsdelivr.net/npm/${packageName}@${version}/${filePath}`"
+              target="_blank"
+              rel="noopener noreferrer"
             >
               {{ $t('code.view_raw') }}
+              <span class="i-lucide:external-link w-3 h-3 inline-block ml-1" aria-hidden="true" />
             </LinkBase>

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4b5f336c-2b3d-46de-aaf1-d539e366668d

📥 Commits

Reviewing files that changed from the base of the PR and between 58da597 and 484e055.

📒 Files selected for processing (4)
  • app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue
  • app/utils/file-types.ts
  • i18n/locales/en.json
  • i18n/schema.json

@43081j
Copy link
Contributor

43081j commented Mar 6, 2026

I'm not sure I would have this "render anyway" option. If the user really wants to look at a binary file, they can "view raw".

This is how GitHub does it too and it does work pretty well

@ShroXd
Copy link
Contributor Author

ShroXd commented Mar 7, 2026

I'm not sure I would have this "render anyway" option. If the user really wants to look at a binary file, they can "view raw".

This is how GitHub does it too and it does work pretty well

Hi, thanks for the feedback!

That's a good point! I checked the behavior on Github, it does look good. I will update my code.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 698b1378-45f1-45d6-bf35-f1b53972e13f

📥 Commits

Reviewing files that changed from the base of the PR and between 484e055 and b934ca7.

📒 Files selected for processing (3)
  • app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue
  • i18n/locales/en.json
  • i18n/schema.json
🚧 Files skipped from review as they are similar to previous changes (2)
  • i18n/schema.json
  • i18n/locales/en.json

])

export function isBinaryFilePath(filePath: string): boolean {
const ext = filePath.split('.').pop()?.toLowerCase() ?? ''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be more efficient to use lastIndexOf here and, if it is more than -1, slice from there to get the extension

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Package code page shows binary files as a raw string

2 participants