Skip to content
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

Support auto-redirection by <link> #106

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/_locales/en/messages.json
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,10 @@
"message": "Prevents the use of an extra tab and instead redirects the action on the main page.",
"description": "This is an option shown in the add-on settings."
},
"optionRedirectImmediately": {
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks a lot for introducing a setting!

IMHO, for a good UX we should then have two settings (both being enabled by default I guess):

  • one like this here enabling the "instant redirect" and
  • one covering the "old" redirects

This would enable one to disable the old redirect style (e.g. because of missing features/unwanted redirects like #28).

"message": "Redirect immediately on page load, instead of on action.",
"description": "This is an option shown in the add-on settings."
},
"translatorCredit": {
"message": "This add-on has been translated into English by $TRANSLATORS$.",
"description": "The credit text for the translator. See https://github.com/TinyWebEx/common/blob/master/CONTRIBUTING.md#translator-credit-inside-of-add-on for how to translate this.",
Expand Down
46 changes: 46 additions & 0 deletions src/background/modules/ActivityPubRedirect.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import * as NetworkTools from "/common/modules/NetworkTools.js";
import * as AddonSettings from "/common/modules/AddonSettings/AddonSettings.js";
import { resolveActivityPubId } from "/common/modules/MastodonApi.js";

/**
* Scrapes the ActivityPub <link> destination from the HTML page, if needed and exists.
*
* @param {number} tabId
* @param {URL} url
* @returns {Promise}
*/
export async function redirectByActivityPubLink(tabId, url) {
if (!tabId || !url) {
throw new Error("Needs a tab id and a page URL");
}

const [objectId] = await browser.tabs.executeScript(tabId, {
code: `document.querySelector("link[rel=alternate][type='application/activity+json']")?.href`,
runAt: "document_end",
});
if (!objectId) {
return;
}

const ownMastodon = await AddonSettings.get("ownMastodon");
if (ownMastodon.server === url.hostname) {
return;
}

const body = await resolveActivityPubId(ownMastodon.server, objectId);

const baseUrl = `https://${ownMastodon.server}`;
let homeUrl;
if (body.accounts[0]) {
homeUrl = new URL(`@${body.accounts[0].acct}`, baseUrl);
} else if (body.statuses[0]) {
homeUrl = new URL(
`@${body.statuses[0].account.acct}/${body.statuses[0].id}`,
baseUrl
);
} else {
return;
}

await NetworkTools.redirectToWebsite(homeUrl, tabId);
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
await NetworkTools.redirectToWebsite(homeUrl, tabId);
return NetworkTools.redirectToWebsite(homeUrl, tabId);

Any special reason you use await instead of just returning the Promise? That could be helpful for error handling or so, that's why I'd prefer return if there is no reason.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm? I believe awaiting is more helpful, since an error will throw at await call site but not at return call site. And awaiting is anyway more consistent.

Copy link
Owner

Choose a reason for hiding this comment

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

hmm this goes deep into the way promises may be handled and I would have said before it makes no difference at all whether to await here, because likely where this is called it is also awaited. And await is just a cooler syntax for then.

But what you say may be right too, the stacktrace may not lost thus last function then? But I am really unsure here, so a js snippet/proof or some source would help. I am always open to learn something new hehe. Otherwise I'd stand to what I said, I.e. remove the await, because that would be consistent to how it is done in the code base everywhere else.

}
9 changes: 8 additions & 1 deletion src/background/modules/AutoRemoteFollow.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import * as MisskeyDetect from "./Detect/Misskey.js";

import * as NetworkTools from "/common/modules/NetworkTools.js";
import * as MastodonRedirect from "./MastodonRedirect.js";
import { redirectByActivityPubLink } from "./ActivityPubRedirect.js";
Copy link
Owner

Choose a reason for hiding this comment

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

BTW I usually prefer a "module-like style", i.e. import * and then do a ActivityPubRedirect.redirectByActivityPubLink() call that makes clear where the module is from.


import * as AddonSettings from "/common/modules/AddonSettings/AddonSettings.js";
import * as MastodonHandleCheck from "/common/modules/MastodonHandle/ConfigCheck.js";
Expand Down Expand Up @@ -149,12 +150,18 @@ function getInteractionType(url) {

/**
* Handles changes to the URL of a tab.
*
*
* @param {string} tabId
* @param {Object} changeInfo
* @returns {void}
*/
async function onTabUpdate(tabId, changeInfo) {
// clear cache of settings
await AddonSettings.loadOptions();
if (await AddonSettings.get("redirectImmediately")) {
redirectByActivityPubLink(tabId, changeInfo.url);
Copy link
Owner

Choose a reason for hiding this comment

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

not awaiting this is deliberate?

Maybe put a commend to make that clear…

}

const ownMastodon = await AddonSettings.get("ownMastodon");
const currentURL = new URL(changeInfo.url);
if (ownMastodon.server !== currentURL.hostname){
Expand Down
19 changes: 19 additions & 0 deletions src/common/modules/MastodonApi.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,22 @@ export function getTootStatus(mastodonServer, localTootId) {
return response.json();
});
}

/**
* Resolves the ActivityPub object ID on a given server
Copy link
Owner

@rugk rugk Jun 20, 2023

Choose a reason for hiding this comment

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

any link/ref to the ActivityPub spec to add to this doc to exlain what it is? I've found one before, but I don't know whether that is up-to-date.

*
* @param {string} mastodonServer
* @param {string} objectId
*/
export async function resolveActivityPubId(mastodonServer, objectId) {
const apiUrl = new URL("api/v2/search", `https://${mastodonServer}`);
apiUrl.searchParams.set("q", objectId);
apiUrl.searchParams.set("resolve", "true");

const response = await NetworkTools.fetch(apiUrl);
if (!response.ok) {
throw new MastodonApiError(mastodonServer, response, `Failed to resolve ${objectId}.`);
}

return await response.json();
}
3 changes: 2 additions & 1 deletion src/common/modules/data/DefaultSettings.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@

export const DEFAULT_SETTINGS = Object.freeze({
ownMastodon: null,
redirectInMainWindow: false
redirectInMainWindow: false,
redirectImmediately: false,
Copy link
Owner

Choose a reason for hiding this comment

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

Why not enabled by default?

});
4 changes: 4 additions & 0 deletions src/options/options.html
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,10 @@
<img class="icon-dismiss invisible" src="/common/img/close.svg" width="24" height="24" tabindex="0" data-i18n data-i18n-aria-label="__MSG_dismissIconDescription__"></span>
</div>
</div>
<div class="line indent">
<input class="setting save-on-change" type="checkbox" id="redirectImmediately" name="redirectImmediately">
<label data-i18n="__MSG_optionRedirectImmediately__" for="redirectImmediately">Redirect immediately on page load, instead of on action.</label>
</div>
</li>
</ul>

Expand Down