Page MenuHomePhabricator

Conpherence - add support for linking directly to messages regardless of age of message
ClosedPublic

Authored by btrahan on Apr 30 2015, 9:55 PM.

Details

Summary

Fixes T7757. Since anchor links can't be processed server side, we have to detect the message is old in javascript, then re-loaded the page. This opens up a new corner case where we have to paginate in newer messages, so this also adds support for that.

Test Plan
  • set main query limit to 8 and then visited ZXX#YYY. noted a second quick load of YYY, that YYY ended up highlighted and scrolled to.
  • used "show newer messages" and "show older messages" successfully, taking care to make sure transaction ids were all correct with no off by one errors, etc.
  • opened and closed durable column to make sure that still works too

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

btrahan retitled this revision from to Conpherence - add support for linking directly to messages regardless of age of message.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.

Hmm, got this when opening the durable column:

btrahan edited edge metadata.

make sure to add newest_transaction_id to durable column call site (Note its always 0 in durable column, but thats cool.)

epriestley edited edge metadata.

I can't actually get "Show Newer Messages" to show up -- when I follow a link to an old message, it just has a bunch of messages after it and then ends abruptly, somewhere further back in history than the present state. That said, this generally works fine.

I wonder if we maybe shouldn't just link to some kind of separate, non-interactive, permanent, paginated view for this and for search results. If I follow a link to #1 and the thread has 1 million messages in it, I assume nothing good happens if I try to add a new message. If we just took you to a separate history view instead that might simplify things.

Maybe I'll try building that for search (I don't want to try dealing with search + new messages interacting for v0 at least, anyway) and we can see if it feels too weird/separate or not.

This revision is now accepted and ready to land.Apr 30 2015, 11:13 PM

"Show Newer Messages" should work... Are you messing with the limit at all or just happen to have 100+ messages in a thread to do this the right way? I'd like to resolve how come its not working for you as I think its kind of important.

I tried building this a few ways, including having a dialogue that was like "This message is old. Here it is with some context <$messages> Should we load the thread around then?"

...but basically it seems to me the expectation would be to drop right into the thread where ever the link linked you to.

...never mind, now I repro too.

I spammed a pile of legit messages, here's what I see when I follow a link with a hashtag to a very early one, which works fine:

When I scroll to the bottom, I see this:

But that's not the most recent message. If I go to the thread directly (with no hash) I get this:

btrahan edited edge metadata.

Fix issue with "show newer messages" not showing up.

There was a bug where if you picked an old message that was within the first OLDER_FETCH_LIMIT messages, we wouldn't fetch enough total messages to trigger the overall limit conditional. As such, add a special case to just render the "show newer messages" transaction if we don't hit the total transaction limit.

If I follow a link to #1 and the thread has 1 million messages in it, I assume nothing good happens if I try to add a new message. If we just took you to a separate history view instead that might simplify things.

This is still an outstanding issue even with the patch.

This revision was automatically updated to reflect the committed changes.