-
Notifications
You must be signed in to change notification settings - Fork 490
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(files): prefer subdomain gw in copied share links #2255
Conversation
… gateway support, and refactored tests.
Fix issue ipfs#2244 Share Link incorrectly gives path routed instead of subdomain routed URL
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @acul71, this is very useful improvement that allows users to avoid hitting HTTP redirect on page load.
Did a quick first-pass review, small asks below.
raw block without dag-pb envelope, saves us 8 bytes Co-authored-by: Marcin Rataj <lidel@lidel.org>
reuse const Co-authored-by: Marcin Rataj <lidel@lidel.org>
this is GUI app, let's keep it short and avoid technical jargon Co-authored-by: Marcin Rataj <lidel@lidel.org>
prefer simpler, single sentence. Co-authored-by: Marcin Rataj <lidel@lidel.org>
…s, refactored image check code, and simplified settings text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few nits and one change that I think will simplify the new form/input a bit (removing showFailState
)
src/components/public-subdomain-gateway-form/PublicSubdomainGatewayForm.js
Outdated
Show resolved
Hide resolved
src/components/public-subdomain-gateway-form/PublicSubdomainGatewayForm.js
Outdated
Show resolved
Hide resolved
src/components/public-subdomain-gateway-form/PublicSubdomainGatewayForm.js
Outdated
Show resolved
Hide resolved
src/components/public-subdomain-gateway-form/PublicSubdomainGatewayForm.js
Outdated
Show resolved
Hide resolved
src/components/public-subdomain-gateway-form/PublicSubdomainGatewayForm.js
Outdated
Show resolved
Hide resolved
remove debug code Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
…tewayForm.js Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
…tewayForm.js Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
…tewayForm.js Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
…tewayForm.js Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
…tewayForm.js Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, two minor changes requested. will wait to merge for @lidel to take a peek
Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
Co-authored-by: Russell Dempsey <1173416+SgtPooki@users.noreply.github.com>
@lidel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, works locally..
we do not need to probe default dweb.link every time, doing it for custom gateways is enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## [4.3.1](v4.3.0...v4.3.1) (2024-09-23) CID `bafybeideglc722hiwhsy4kiyl2fivf5lr6wozy2iuixtgzkvl3v4hasaty` --- ### Bug Fixes * **file-preview:** safeSubresourceGwUrl ([#2253](#2253)) ([bb861a3](bb861a3)), closes [/github.com//issues/2246#issuecomment-2322192398](https://github.com/ipfs//github.com/ipfs/ipfs-webui/issues/2246/issues/issuecomment-2322192398) * **files:** avoid duplicated fetch during preview ([#2254](#2254)) ([eefae25](eefae25)), closes [#2217](#2217) * **files:** prefer subdomain gw in copied share links ([#2255](#2255)) ([e8c4421](e8c4421)) ### Trivial Changes * .io → .tech ([b9f622d](b9f622d)) * browserslist@latest ([#2251](#2251)) ([809c55a](809c55a)) * Pull transifex translations ([#2258](#2258)) ([2250374](2250374)) * use ipld-explorer-components@7.0.2 ([#2257](#2257)) ([99ba9ac](99ba9ac))
🎉 This PR is included in version 4.3.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Issue #2244
When you right click on a file in Files and choose "Share Link" you are given a URL like
https://<host>/ipfs/<cid>
. This should be of the formhttps://<cid>.ipfs.<host>
for security reasons.Solution
Modify the setting page and relevant code so that the user can adjust both subdomain and path gateway they use, but by default we use subdomain whenever possible
Code
getShareableLink
function insrc/lib/files.js
it nowGenerates a shareable link for the provided files using a subdomain gateway as default or a path gateway as fallback
src/bundles/gateway.js
a New functioncheckSubdomainGateway (gatewayUrl)
that Checks if a given gateway URL is functioning correctly by verifying image loading and redirection.settings/SettingsPage.js
and labels inpublic/locales/en/app.json
andpublic/locales/en/settings.json
##############
Tests: settings page
I've added a couple of tests in
test/e2e/settings.test.js
for submit/reset functionality of public gatewayAnd another test
src/lib/files.test.js
for checking subdomain and path gateway urls