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

Link to conversation should be robust against renaming #11699

Closed
opensource21 opened this issue Feb 27, 2019 · 7 comments
Closed

Link to conversation should be robust against renaming #11699

opensource21 opened this issue Feb 27, 2019 · 7 comments

Comments

@opensource21
Copy link

A link to a conversation becomes broken if the topic is changed. There are two things that should be changed:

  1. change subject in url to topic for a consistent naming
  2. Change the URL-Resolving behaviour, so that it looks only to the id and adjust the stream and the topic.
    See zulip-chat
@zulipbot
Copy link
Member

zulipbot commented Feb 27, 2019

Hello @zulip/server-message-view, @zulip/server-search members, this issue was labeled with the "area: message view", "area: message-editing", "area: search" labels, so you may want to check it out!

@timabbott timabbott added area: message-editing difficult Issues which we expect to be quite difficult labels Feb 27, 2019
@timabbott
Copy link
Member

This is unfortunately a somewhat complex issue. The natural thing to do here is look up the message ID in the URL, and if it matches the stream ID but not the topic, just send you to the other topic. But there's a few problems with doing that generically:

  • The generic /near/ syntax supports passing a message ID that doesn't match the topic (or even stream at all), and we don't want to break/change the existing behavior that you can use /near/1/ to get the very first messages in a thread.
  • The browser likely will not have details on the target message ID present at the time it is picking the narrow, so we might need to do a fetch+narrow-on-callback approach for cases like this.

If we do the callback approach, I think we can inspect the message_edit_history field of the message object we had to fetch anyway (to get the topic) to confirm whether or not the target message ID actually had a topic edit in its past.

@showell
Copy link
Contributor

showell commented Mar 1, 2019

This is definitely a tough issue. I agree with 80% of Tim's prior comment, except that we don't need to look at message_edit_history. The complexity is that we do need to get the message-id on the backend, generally, and do something like fetch-and-narrow.

@showell
Copy link
Contributor

showell commented Mar 1, 2019

The other possibility here is to have only one round trip between the client and server:

  • client sends .../topic/lunch/near/42
  • backend fetches message 42
  • in the unlikely event that message 42 does not exist (a manually build url), continue through normal codepath, and we're done
  • if message 42 has topic "lunch", continue through current codepath, and we're done
  • if message 42 has topic "dinner", change filter to dinner, get data from DB
  • send back results for .../topic/dinner/near/42 to client

When the client gets results back, it should look at the topic for message 42. If the topics changed, it should reflect that properly in the left sidebar, and probably the search bar and URL as well.

@timabbott
Copy link
Member

I feel like this was add a significantly different flow; I had been imagining just having the hashchange logic check if it has the message ID cached locally; if it does, do the processing there, otherwise fetch that message ID from the server and have a success handler to do the narrow once we have the message returned.

@vrongmeal
Copy link
Collaborator

I had a doubt. Is it mentioned specifically that if you, say go to the URL with /near/1, it will take you to the first message? (I thought it might work for near 2, 3 ... and so on but it's just that it fetches the stream and topic and if no message id matches the number, it just takes you to the top, i.e., the first message).

@timabbott
Copy link
Member

Closing in favor of #21505.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants