Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix issue where thread panel did not update correctly #9746

Merged
merged 1 commit into from
Dec 13, 2022
Merged

Fix issue where thread panel did not update correctly #9746

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/structures/ThreadPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ const ThreadPanel: React.FC<IProps> = ({ roomId, onClose, permalinkCreator }) =>
<Measured sensor={card.current} onMeasurement={setNarrow} />
{timelineSet ? (
<TimelinePanel
key={timelineSet.getFilter()?.filterId ?? roomId + ":" + filterOption}
key={filterOption + ":" + (timelineSet.getFilter()?.filterId ?? roomId)}
Copy link
Contributor

@MadLittleMods MadLittleMods Dec 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Seems like we should add a test for the problem that this fixes. We could add a test to ensure the filtered view shows a subset of what all threads shows.

Was there a React warning about duplicate keys being used? Can we fail tests if this ever occurs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do have a warning for this, it's the "timeline panel being reused with a different timelineset" warning. I don't think there's an easy way to reliably automate testing this, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could add a test to ensure the filtered view shows a subset of what all threads shows.

What about a test for this scenario ^?

We do have a warning for this, it's the "timeline panel being reused with a different timelineset" warning.

Where is this warning emitted? Do you mean Replacing timelineSet on a TimelinePanel - confusion may ensue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's exactly the warning I meant. Sorry, I'm on a personal device right now, going by memory.

I'll reply in more detail later when I'm back at work.

Copy link
Contributor

Choose a reason for hiding this comment

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

@justjanne Friendly poke

We can write an end-to-end Cypress test for this particular case which avoids the internal hell implementation and the current test harness for it.

If the Replacing timelineSet on a TimelinePanel - confusion may ensue warning is always considered a bug when we hit that point, we should do more to ask for a bug report when it happens. We can show a bug report suggestion in the thread view and explain that something went wrong.

ref={timelinePanel}
showReadReceipts={false} // No RR support in thread's list
manageReadReceipts={false} // No RR support in thread's list
Expand Down