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

add 'hidden' tab in profile page, listing all blocked addresses in account file #356

Open
plebeius-eth opened this issue May 30, 2024 · 10 comments
Assignees
Labels
enhancement New feature or request

Comments

@plebeius-eth
Copy link
Member

No description provided.

@plebeius-eth plebeius-eth self-assigned this May 30, 2024
@plebeius-eth plebeius-eth converted this from a draft issue May 30, 2024
@plebeius-eth plebeius-eth added the enhancement New feature or request label May 30, 2024
@plebeius-eth plebeius-eth moved this from Todo to In Progress in seedit Jun 1, 2024
@plebeius-eth
Copy link
Member Author

911fa76

@github-project-automation github-project-automation bot moved this from In Progress to Done in seedit Jun 2, 2024
@estebanabaroa
Copy link
Member

estebanabaroa commented Jun 10, 2024

const Profile = () => {
  const { t } = useTranslation();
  const account = useAccount();
  const location = useLocation();
  const params = useParams();
  let { accountComments } = useAccountComments();
  accountComments = [...accountComments].reverse();
  const { accountVotes } = useAccountVotes();
  const isInProfileUpvotedView = isProfileUpvotedView(location.pathname);
  const isInProfileDownvotedView = isProfileDownvotedView(location.pathname);
  const isInProfileHiddenView = isProfileHiddenView(location.pathname);
  const isInCommentsView = isProfileCommentsView(location.pathname);
  const isInSubmittedView = isProfileSubmittedView(location.pathname);
  const isMobile = useWindowWidth() < 640;

  // get comments for upvoted/downvoted/comments/submitted pages
  const postComments = useMemo(() => accountComments?.filter((comment) => !comment.parentCid) || [], [accountComments]);
  const replyComments = useMemo(() => accountComments?.filter((comment) => comment.parentCid) || [], [accountComments]);
  const upvotedCommentCids = useMemo(() => accountVotes?.filter((vote) => vote.vote === 1).map((vote) => vote.commentCid) || [], [accountVotes]);
  const downvotedCommentCids = useMemo(() => accountVotes?.filter((vote) => vote.vote === -1).map((vote) => vote.commentCid) || [], [accountVotes]);
  const hiddenCommentCids = useMemo(() => Object.keys(account?.blockedCids ?? {}), [account?.blockedCids]);

  const { comments: upvotedComments } = useComments({ commentCids: upvotedCommentCids });
  const { comments: downvotedComments } = useComments({ commentCids: downvotedCommentCids });
  const { comments: hiddenComments } = useComments({ commentCids: hiddenCommentCids });

this will destroy performance, every time the user goes to the profile page, even if he doesnt want to see his hidden posts, all the hidden posts will be loaded one by one, it will queue hundreds or thousands of ipfs gateway requests and the user won't be able to load anything else.

in general you should never use useComments with more than maybe 10 comments at a time. because it does 1 request for each comment, and they are super slow, and they clog up the maximum amount of ipfs gateway requests that the browser can do and it makes it impossible to load the rest of the app.

so what I suggest is to make a component <Upvoted> <Downvoted> <Hidden> that is only rendered when the user is on the correct route, and it should be paginated to only load 10 comments at a time (10 per page) using useComments, I wouldnt use infinite scroll, I would do profile/upvoted/page/2, you cannot pass all comments, there could be thousands, it will break the app to try to load all of them at once.

in general you should never use useComments, it has extremely bad performance. if you have to use it, like for example for the plebchan home page, or to load hidden/upvoted/downvoted, it should only be done with less than 10 comments at a time, and it should only be done if the user actually visits the page, it should never be done for no reason as it will render the web app unusable

@estebanabaroa estebanabaroa reopened this Jun 10, 2024
@estebanabaroa estebanabaroa moved this from Done to Todo in seedit Jun 10, 2024
@estebanabaroa
Copy link
Member

estebanabaroa commented Jun 10, 2024

also I think you can use <Routes> inside the view, I dont think you need to create your own routing components like isProfileUpvotedView(). I'm not 100% sure.

or it might just be easier to create a new view for hidden, upvoted and downvoted. the code would be mostly duplicated but I don't think it matters much if you're just importing a few reusable components from components/

@plebeius-eth plebeius-eth removed the status in seedit Jun 10, 2024
@plebeius-eth plebeius-eth moved this to In Progress in seedit Sep 9, 2024
@plebeius-eth
Copy link
Member Author

refactored 9434a17

@github-project-automation github-project-automation bot moved this from In Progress to Done in seedit Oct 23, 2024
@estebanabaroa
Copy link
Member

what is that

const CheckRouteParams = () => {
  const { accountCommentIndex, sortType, timeFilterName } = useParams();
  const isValidAccountCommentIndex = !accountCommentIndex || (!isNaN(parseInt(accountCommentIndex)) && parseInt(accountCommentIndex) >= 0);
  const isSortTypeValid = !sortType || sortTypes.includes(sortType);
  const isTimeFilterNameValid = !timeFilterName || timeFilterNames.includes(timeFilterName as any);
  const isAccountCommentIndexValid = !accountCommentIndex || !isNaN(parseInt(accountCommentIndex));

  if (!isValidAccountCommentIndex || !isSortTypeValid || !isTimeFilterNameValid || !isAccountCommentIndexValid) {
    return <NotFound />;
  }

  return <Outlet />;
};

why cant it use react-router normally?

you shouldnt code routing manually, you should use the library as its meant to be used, unless it's missing some feature I dont know about

              <Route path='/profile' element={<Profile />}>
                <Route index element={<Profile.Overview />} />
                <Route path='upvoted' element={<Profile.VotedComments voteType={1} />} />
                <Route path='downvoted' element={<Profile.VotedComments voteType={-1} />} />
                <Route path='hidden' element={<Profile.HiddenComments />} />
                <Route path='comments' element={<Profile.Comments />} />
                <Route path='submitted' element={<Profile.Submitted />} />
              </Route>

why is this code in app.tsx? it should be in views/profile.tsx

views should only export 1 file, the view, it shouldnt export individual components

you can use inside views, you dont have to do all the routing inside app.tsx

profile.tsx can do its own subrouting using

@estebanabaroa estebanabaroa reopened this Oct 27, 2024
@plebeius-eth
Copy link
Member Author

why cant it use react-router normally?

I don't want to use or something to redirect to , because I want the NotFound view to get routed both with a wildcard and with the custom hook. The hook useValidateRouteParams (prev. CheckRouteParams) checks dynamic params, for example: if a user has posted 3 times only, profile/1 will go to Profile, profile/100 will go to NotFound

I don't know if there's a better way to do this. I agree the hook looked confusing so I moved it in the hooks directory.

f2467cb

@plebeius-eth
Copy link
Member Author

it should be in views/profile.tsx

it can't, because we use profile/:accountCommentIndex as route for the pending post page, which has to be validated by useValidateRouteParams hook to prevent the user from manually going to an invalid accountCommentIndex, redirecting him to NotFound instead. This is why we can't use a profile/* wildcard in app.tsx, which would be needed if we were to move all the profile page routing to profile.tsx.

@estebanabaroa
Copy link
Member

estebanabaroa commented Nov 13, 2024

this can't be done imo:

const ValidatedRoute = () => {
  const isValid = useValidateRouteParams();

  if (!isValid) {
    return <NotFound />;
  }

  return <Outlet />;
};
<Route element={<ValidatedRoute />}>

if we want to display an error when the profile page has no posts, we should just display an error from inside the profile page. it's not necessary to do anything weird to the routing. also the user will almost never land on profile/<post doesnt exist> so we shouldnt make the code uglier and potentially buggy/less performant just to handle this use case that almost never happens.

we shouldnt implement our own routing ever imo, there's a reason we're using a routing library, it's clean, it's documented, it's self explanatory to people who read the code, it won't cause bugs, it wont have random performance issues and it wont be difficult to edit later.

adding random pieces of routing for no valid reason (the error can be handled in profile page) and using react-router in an undocumented and weird way is really bad, I am confused by that code and I wouldnt be able to add new functionality to it without being confused. I'm also not sure using react-router this way wont cause performance issues, we dont really know how many times this thing is rerendering and why/when. routing is special and low level, it has to be extremely fast cause it's done over and over and it's done before anything else happens.

@estebanabaroa estebanabaroa reopened this Nov 13, 2024
@estebanabaroa estebanabaroa moved this from Done to Todo in seedit Nov 13, 2024
@estebanabaroa
Copy link
Member

estebanabaroa commented Nov 13, 2024

it can't, because we use profile/:accountCommentIndex as route for the pending post page, which has to be validated by useValidateRouteParams hook to prevent the user from manually going to an invalid accountCommentIndex, redirecting him to NotFound instead. This is why we can't use a profile/* wildcard in app.tsx, which would be needed if we were to move all the profile page routing to profile.tsx.

imo we have to remove useValidateRouteParams and display a custom error in the profile page. maybe it would be slightly better UX to display the regular "not found" page, but it's an error that happens so rarely that imo it's not worth it to make the routing messy and confusing just for that.

also you could argue that displaying a custom error for profile/<invalid post> might be better. like we could figure out how the user ever landed on this page, and either redirect him to the correct page, or tell him specifically what he did wrong. even if that's not a 1:1 copy of reddit, it doesnt really matter cause it's something that almost never happens, like someone could use seedit for years and never see this error, so we're not really losing immersion.

the rule of thumb imo is we want to be almost 1:1 reddit UI for the core experience, stuff that people do over and over thousands of time, like the feed and post page, but for stuff that people do 0.00001% of the time, we don't really have to be 1:1, it doesn't matter that much.

@estebanabaroa
Copy link
Member

estebanabaroa commented Nov 13, 2024

also another way we could do it is:

  if (isNotFound) {
    return <NotFound />
  }

  return (
    <div className={`${styles.app} ${theme}`}>
      <Routes>
        // ....

this way we don't have to make the Profile page export components, views should just export 1 view (1 component) as a convention, if we start doing stuff randomly, it becomes really messy.

also I dont think we should do that, cause that would be a form of implementing our own routing, and we have to wait for useAccountComments() to calculate every time, and it might cause rerenders, etc. imo it's not worth it just to be able to send the NotFound component in this really rare scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Todo
Development

No branches or pull requests

2 participants