Change public accounts pages to mount the web UI #19319

Gargron · 2022-10-08T08:51:46Z

Remove /web prefix from web app (maintain redirect)
Redirect any non-local resource to its permalink (accounts, statuses)
Pre-render a noindex tag on all web app pages except:
- Profiles
- Statuses
- About
- Privacy Policy
- Explore (while it's rendered through the HomeController, we make an exception when request.path == '/')
Override the noindex tag in the web app for the same pages when a search engine navigates within the web app
Ensure that the web app renders a noindex tag on error pages, including error boundary
Remove remote follow / remote interaction dialogs (superceded by in-app interaction modal)

This finally completes #16800

Note about robots meta tag handling: react-helmet does not overwrite header tags that were already in the HTML, it only appends new ones. That means if the HTML sets one robots tag, it will be present on any subsequent SPA navigation, even if other routes append their own robots tag. I am not 100% sure how such a case is handled by search engines -- which tag is prioritized, the first, or the last. That being said, according to Google's documentation, Google will only render and process the page's JavaScript if the page does not have a noindex directive on it. That means we only have to worry about SPA-rendered robots tags if the initial tag was not noindex.

ykzts

git rm spec/controllers/remote_follow_controller_spec.rb

ClearlyClaire · 2022-10-15T11:41:57Z

Once again, please do break such PRs in multiple PRs or at least multiple commits.

Not by any means a full review, but I can see at least two outstanding issues with this PR:

on my development environment, the advanced interface is broken with https://reactjs.org/docs/error-decoder.html?invariant=185 ; this seems to be caused by BundleColumnError using ColumnHeader without passing down the multiColumn property as appropriate (in my case, BundleColumnError is used because i had a pinned group column in my dev environment, but the point of BundleColumnError is to not crash the whole UI on error anyway)
profiles for remote users are routed client-side but not server-side: if you click on a remote user, say @foo@bar, the URL is https://instance.com/@foo@bar and refreshing will lead to a 404

- When logged in, serve web app - When logged out, redirect to permalink - Fix `app-body` class not being set sometimes due to name conflict

ClearlyClaire

I can confirm the crash has been fixed. The routing situation is better, but still leads to the awkward situation that you can navigate to /@foo@bar (a remote account displayed in the WebUI) or /@foo@bar/1234 (a remote status displayed in the WebUI) from the logged-out WebUI but refreshing the page (or re-loading an unloaded page) will unexpectedly redirect you to the remote server.

I'm not sure what the best solution is here, as all the alternatives I can imagine have their own drawback:

not redirecting allows sharing links to the cached version of a post
opening the remote content in a new tab rather than allowing navigation to the cached version of it may not be as smooth as the current logged-out UI is. But maybe it's not so bad, that's what we had until you changed the logged-out UI to use the WebUI

Apart from that, I'm confused about some of the noindex uses. I have added inline comments.

I still have more general reservations about getting rid of server-side-rendered pages, in that:

pages that did not require Javascript now require a good amount of it being loaded before showing anything. This means longer loading times, and it also means people disabling Javascript or with an incompatible browser (while we strive to maintain compatibility with a large amount of browsers, incompatibilities do occur) just won't see posts that people may share with them
while I do think this reduces confusion about there being two views for you on your own server, I'm afraid it would bring more confusion when visiting remote servers. But maybe I'm wrong about that, and I'm not sure there's any way to know for sure except trying it at scale…

ClearlyClaire · 2022-10-17T08:42:11Z

app/controllers/about_controller.rb

+  before_action :set_instance_presenter
+
  def show
    expires_in 0, public: true unless user_signed_in?
  end
+
+  private
+
+  def set_instance_presenter
+    @instance_presenter = InstancePresenter.new
+  end


How is this related?

There was a crash because this presenter is used by the OpenGraph partial

ok, but I'm not sure how adding the OpenGraph partial to this controller is related to begin with

ClearlyClaire · 2022-10-17T09:09:30Z

app/javascript/mastodon/components/missing_indicator.js

+
+    <Helmet>
+      <meta name='robots' content='noindex' />
+    </Helmet>


Isn't the MissingIndicator always used while status and accounts haven't loaded yet? In this case, this would cause pages to never get indexed.

Good point, loading indicator for the status page would be a good thing...

app/javascript/mastodon/containers/mastodon.js

ClearlyClaire · 2022-10-17T10:58:41Z

app/models/account.rb

@@ -134,6 +134,7 @@ class Account < ApplicationRecord
           :role,
           :locale,
           :shows_application?,
+           :prefers_noindex?,


This name sounds slightly weird to me as it's not that you prefer noindex, it's that you want noindex, but ok.

I used this wording because there's no actual way to enforce whether a search engine indexes something or not.

Right, but you can ensure you output noindex.

… is mounted

ClearlyClaire

My concerns about requiring Javascript, as well as refreshing the WebUI unexpectedly redirecting to remote URLs remain. The issues with noindex for statuses seem solved, but that's not the case for accounts.

ClearlyClaire

I believe the issues with noindex have been resolved, my only remaining concerns are refreshing the WebUI unexpectedly redirecting to remote URLs, and more general concerns about requiring the use of Javascript.

busterb · 2022-11-18T13:23:44Z

Howdy. Was trying to understand what the 'Feature on profile' option does in the UI, and stumbled across the endorsements API. After some backtracking through the tree, it seems that this change accidentally deleted the visualization from the UI:

839f893#r90342981

Seems this feature is no more as of this change, or it's not in a place I can find it :) : #8146

ClearlyClaire · 2022-11-18T13:24:50Z

Yeah, it's currently not anywhere, unfortunately :/

* Change public accounts pages to mount the web UI * Fix handling of remote usernames in routes - When logged in, serve web app - When logged out, redirect to permalink - Fix `app-body` class not being set sometimes due to name conflict * Fix missing `multiColumn` prop * Fix failing test * Use `discoverable` attribute to control indexing directives * Fix `<ColumnLoading />` not using `multiColumn` * Add `noindex` to accounts in REST API * Change noindex directive to not be rendered by default before a route is mounted * Add loading indicator for detailed status in web UI * Fix missing indicator appearing while account is loading in web UI

* Change public accounts pages to mount the web UI * Fix handling of remote usernames in routes - When logged in, serve web app - When logged out, redirect to permalink - Fix `app-body` class not being set sometimes due to name conflict * Fix missing `multiColumn` prop * Fix failing test * Use `discoverable` attribute to control indexing directives * Fix `<ColumnLoading />` not using `multiColumn` * Add `noindex` to accounts in REST API * Change noindex directive to not be rendered by default before a route is mounted * Add loading indicator for detailed status in web UI * Fix missing indicator appearing while account is loading in web UI # Conflicts: # app/serializers/rest/account_serializer.rb # app/views/statuses/_detailed_status.html.haml # app/views/statuses/_simple_status.html.haml

ncfavier · 2022-11-26T09:19:35Z

Remove remote follow / remote interaction dialogs (superceded by in-app interaction modal)

I don't understand this at all. The new interaction modal requires one to open a new tab/window to their own instance, copy the given URL, paste it into the search box before they can interact with any post. This is much, much less practical than the old modals that just asked me for my handle and then remembered it, so that any interaction takes two clicks.

Please at least provide a way to revert to the old behaviour, which should be the default IMO.

Gargron added the ui Front-end, design label Oct 8, 2022

Gargron force-pushed the feature-accounts-web-ui branch 9 times, most recently from 4c79d60 to 373ef76 Compare October 15, 2022 10:46

Gargron marked this pull request as ready for review October 15, 2022 10:46

ykzts approved these changes Oct 15, 2022

View reviewed changes

ykzts requested changes Oct 15, 2022

View reviewed changes

Change public accounts pages to mount the web UI

f7a1399

Gargron force-pushed the feature-accounts-web-ui branch from 373ef76 to f7a1399 Compare October 15, 2022 11:03

ykzts approved these changes Oct 15, 2022

View reviewed changes

ClearlyClaire self-requested a review October 15, 2022 11:25

Gargron added 5 commits October 16, 2022 09:20

Fix handling of remote usernames in routes

b55df38

- When logged in, serve web app - When logged out, redirect to permalink - Fix `app-body` class not being set sometimes due to name conflict

Fix missing multiColumn prop

3153fd3

Fix failing test

11f7f7f

Use discoverable attribute to control indexing directives

71b122c

Fix <ColumnLoading /> not using multiColumn

d37df7f

Gargron force-pushed the feature-accounts-web-ui branch from f8ee184 to d37df7f Compare October 16, 2022 08:09

Add noindex to accounts in REST API

fb1fab3

ClearlyClaire reviewed Oct 17, 2022

View reviewed changes

Gargron added 2 commits October 18, 2022 19:21

Change noindex directive to not be rendered by default before a route…

95b0de1

… is mounted

Add loading indicator for detailed status in web UI

762fd33

ClearlyClaire reviewed Oct 19, 2022

View reviewed changes

Fix missing indicator appearing while account is loading in web UI

ce7d49c

ClearlyClaire reviewed Oct 20, 2022

View reviewed changes

Gargron merged commit 839f893 into main Oct 20, 2022

Gargron deleted the feature-accounts-web-ui branch October 20, 2022 12:35

nametoolong mentioned this pull request Nov 20, 2022

Fix N+1 queries in NotificationsController #21202

Merged

ClearlyClaire mentioned this pull request Dec 1, 2022

Fix regression: Verifyable link to new account after profile moved #21844

Closed

c960657 mentioned this pull request Aug 18, 2023

Remove dead code from public.jsx #26547

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change public accounts pages to mount the web UI #19319

Change public accounts pages to mount the web UI #19319

Gargron commented Oct 8, 2022 •

edited

Loading

ykzts left a comment

ClearlyClaire commented Oct 15, 2022 •

edited

Loading

ClearlyClaire left a comment

ClearlyClaire Oct 17, 2022

Gargron Oct 17, 2022

ClearlyClaire Oct 17, 2022

ClearlyClaire Oct 17, 2022

Gargron Oct 17, 2022

ClearlyClaire Oct 17, 2022

Gargron Oct 17, 2022

ClearlyClaire Oct 17, 2022

ClearlyClaire left a comment

ClearlyClaire left a comment

busterb commented Nov 18, 2022

ClearlyClaire commented Nov 18, 2022

ncfavier commented Nov 26, 2022

Change public accounts pages to mount the web UI #19319

Change public accounts pages to mount the web UI #19319

Conversation

Gargron commented Oct 8, 2022 • edited Loading

ykzts left a comment

Choose a reason for hiding this comment

ClearlyClaire commented Oct 15, 2022 • edited Loading

ClearlyClaire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ClearlyClaire left a comment

Choose a reason for hiding this comment

ClearlyClaire left a comment

Choose a reason for hiding this comment

busterb commented Nov 18, 2022

ClearlyClaire commented Nov 18, 2022

ncfavier commented Nov 26, 2022

Gargron commented Oct 8, 2022 •

edited

Loading

ClearlyClaire commented Oct 15, 2022 •

edited

Loading