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

recent_view: Fix no topic found at center of recent view. #30167

Merged
merged 1 commit into from
May 23, 2024

Conversation

amanagr
Copy link
Member

@amanagr amanagr commented May 22, 2024

The bug is hard to reproduce, but I was able to reproduce with #30134 and used the same strategy to fix it here.

Copy link

sentry-io bot commented May 22, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: web/src/recent_view_ui.ts

Function Unhandled Issue
recenter_focus_if_off_screen Error: Assertion failed recenter_focus_if_off_scr...
Event Count: 20 Affected Users: 18

Did you find this useful? React with a 👍 or 👎

assert(topic_element !== null);
row_focus = $topic_rows.index($(topic_element).closest("tr")[0]);
if (topic_element === null) {
// Last visible element is above the vertical center of the table.
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Can you spell out a bit more what this means / why it is correct? I'm missing some detail here.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, there are 2 possibilities of topic_element being null here.

  • One is if we haven't rendered the list yet, which is fixed by is_waiting_for_revive_current_focus which fixes the sentry error.
  • Other is if there is no row at the vertical center of the recent view but slightly above it. In that case, we focus on the last element. This is just something that I noticed is possible but not entierly sure if it is possible to get into this state.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

OK I wrote a comment trying to explain this and will merge ... but I'm not sure it's entirely correct.

Also, $topic_rows.index($(topic_element).closest("tr")[0]); seems like it's not limited to just conversation rows, but it could fire on a header row : I wonder if we shouldn't be using table_wrapper_element.getBoundingClientRect();, but instead table_wrapper_element.find("tbody").getBoundingClientRect();... I guess find is bad because of nested tables, but you get the idea.

The bug is hard to reproduce, but I was able to reproduce with zulip#30134
and used the same strategy to fix it here.
@zulipbot zulipbot added size: M and removed size: S labels May 23, 2024
@timabbott timabbott enabled auto-merge (rebase) May 23, 2024 17:02
@timabbott
Copy link
Sponsor Member

OK, pushed a revision that I think is a good merge point, even if I'm not entirely certain that we're doing the right thing in some unlikely/theoretical cases there.

@timabbott timabbott disabled auto-merge May 23, 2024 17:39
@timabbott
Copy link
Sponsor Member

Test failure looks like an unrelated infrastructure failure affecting the realm_playground puppeteer test.

@timabbott timabbott merged commit e6c074e into zulip:main May 23, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants