Skip to content

Fix: Unable to select text inside blob - #2458

Merged
Kuuuube merged 1 commit into
yomidevs:masterfrom
dlip:fix-blob
Jul 17, 2026
Merged

Fix: Unable to select text inside blob#2458
Kuuuube merged 1 commit into
yomidevs:masterfrom
dlip:fix-blob

Conversation

@dlip

@dlip dlip commented Jul 9, 2026

Copy link
Copy Markdown

Fixes #2457

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 965c0bddc7

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread dev/data/manifest-variants.json Outdated
Comment on lines +236 to +238
"match_origin_as_fallback"
],
"value": true

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Bump Firefox minimum for this key

This adds match_origin_as_fallback to the Firefox variants while the same generated manifest still advertises browser_specific_settings.gecko.strict_min_version and gecko_android.strict_min_version as 115.0; Firefox only supports this content-script key starting in 128. On Firefox/Firefox Android 115–127 the extension is still considered compatible, but the new blob/data/about injection behavior is unsupported, so the reported blob-reader scenario remains broken for part of the declared support range unless the minimum version is raised or an older-version fallback is used.

Useful? React with 👍 / 👎.

@Kuuuube

Kuuuube commented Jul 9, 2026

Copy link
Copy Markdown
Member

Is there a reason you added this as a modification to half the manifest variants instead of modifying the base manifest

@dlip

dlip commented Jul 10, 2026

Copy link
Copy Markdown
Author

@Kuuuube I was worried about Safari since I couldn't test it, but on further googling it seems fine. I added it to the base

@Kuuuube Kuuuube added the kind/bug The issue or PR is regarding a bug label Jul 11, 2026
@dlip

dlip commented Jul 13, 2026

Copy link
Copy Markdown
Author

@Kuuuube I fixed the unit test, could you please run the workflow again?

@Kuuuube

Kuuuube commented Jul 13, 2026

Copy link
Copy Markdown
Member

The @types/chrome package needs to be bumped.

Also if you want to do a full update on the chrome-manifest-schema.json file, can just copy this in: https://raw.githubusercontent.com/cezaraugusto/chrome-extension-manifest-json-schema/refs/heads/main/manifest/manifest.schema.v3.json.

@dlip

dlip commented Jul 13, 2026

Copy link
Copy Markdown
Author

@Kuuuube I updated the chrome-manifest-schema.json, but upgrading @types/chrome broke a lot of things so I just made a minimal patch to the type so the test would pass

@Kuuuube

Kuuuube commented Jul 13, 2026

Copy link
Copy Markdown
Member

Are you sure you only bumped @types/chrome? And you ran npm install & npm ci after? I did give that a quick test before suggesting it and it should work.

@dlip

dlip commented Jul 13, 2026

Copy link
Copy Markdown
Author

@Kuuuube the errors occur with npm run test:ts:main

Errors  Files
     1  ext/js/background/backend.js:478
     1  ext/js/background/script-manager.js:125
     1  ext/js/display/search-display-controller.js:597
     1  ext/js/extension/environment.js:61
     4  ext/js/pages/settings/anki-controller.js:1213
     2  ext/js/pages/settings/permissions-toggle-controller.js:89
     1  ext/js/pages/settings/recommended-permissions-controller.js:85

@dlip

dlip commented Jul 14, 2026

Copy link
Copy Markdown
Author

@Kuuuube I bumped @types/chrome and fixed the errors, let me know if you want to keep it or not

@Kuuuube Kuuuube left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for bumping that. Looks good. Just one small suggestion.

Looks like npm is down maybe. I'll rerun the tests later.

Comment thread ext/js/background/script-manager.js
@dlip
dlip requested a review from Kuuuube July 15, 2026 02:04
@Kuuuube

Kuuuube commented Jul 15, 2026

Copy link
Copy Markdown
Member

Im going to need you to explain why in the package lock you've edited the source of a package to point away from the official npm registry into a third party private npm listing.

I can give you a pass on it not being malicious since the hash is intact but that is super sketchy.

(This is why tests are failing btw)

Also I dont think any of these packages should be peer dependencies? Unless I'm misunderstanding what that means in npm.

@dlip

dlip commented Jul 16, 2026

Copy link
Copy Markdown
Author

@Kuuuube Yes, sorry about the package lock thing, I have an internal private registry set up in my npm config and it seems to have decided to prioritize that. It's not accessible externally, but I agree it was a good idea for you to raise it since it might have been malicious. I cleaned it out and squashed my changes to be sure its not used.

@Kuuuube
Kuuuube added this pull request to the merge queue Jul 17, 2026
Merged via the queue into yomidevs:master with commit c0c3702 Jul 17, 2026
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug The issue or PR is regarding a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't select text inside blobs

2 participants