fix: missing key on welcome page #13225

yardz · 2021-02-18T20:24:54Z

SUMMARY

There is an error on ActivityTable.tsx

I refactored the component because it was too big and it was difficult to debug (and to make it easier to create tests in the future)
I divided it into a second component, and fixed the key problem.

TEST PLAN

Tests should pass normally. No application behavior has been changed.
The ActivityTable.tsx interface has been maintained, and the file has not been moved.

junlincc · 2021-02-18T20:52:42Z

Thanks for being proactive and cleaning up the code as you move, Bruno! ♥️ @yardz

@pkdotson can you review this PR since you are the owner? thanks!

pkdotson · 2021-02-18T21:41:37Z

superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx

@@ -178,7 +115,13 @@ export default function ActivityTable({
      />
      <>
        {activityData[activeChild]?.length > 0 ? (
-          <ActivityContainer>{renderActivity()}</ActivityContainer>
+          <ActivityContainer>
+            <ActivityTableRow


nit: Does it makes sense to call this ActivityCard ?

I gave this name following the logic "table->row". If you look at ActivityTableRow it receives an array and returns several "cards" inside a Fragment.

I had thought of "ActivityList" (list of cards right?), but I chose to keep it as "ActivityTableRow" as there are some improvements to be made in the props and these components are very related...

TableRow sounds like a row of a table, but it actually contains the whole table content for a selected tab. We should probably rename other XxxTable to simply Recents, Dashboards, Charts and SavedQueries to be consistent with section titles, then the single tab content can be named as RecentsList, DashboardList, etc.

TableRow sounds like a row of a table, but it actually contains the whole table content for a selected tab. We should probably rename other XxxTable to simply Recents, Dashboards, Charts and SavedQueries to be consistent with section titles, then the single table content can be named as RecentsList, DashboardList, etc.

I agree that the names are not good. But I think this is a task for another PR, the purpose of this was just to solve the problem of the keys and not to solve "all problems" related to these files.

Example: These files also need test files, the types need to be better defined, etc.
None of this is the scope of this PR, just as renaming the "ActivityTable" does not seem to me to be scope for this PR.

@ktmud

ktmud

bug.mp4

The tab switching ends up with infinite cards bug is back. Plus I'm still seeing the React child key missing error that this PR is supposed to fix.

superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx

ktmud · 2021-02-22T08:34:58Z

superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx

@@ -178,7 +115,13 @@ export default function ActivityTable({
      />
      <>
        {activityData[activeChild]?.length > 0 ? (
-          <ActivityContainer>{renderActivity()}</ActivityContainer>
+          <ActivityContainer>
+            <ActivityTableRow


TableRow sounds like a row of a table, but it actually contains the whole table content for a selected tab. We should probably rename other XxxTable to simply Recents, Dashboards, Charts and SavedQueries to be consistent with section titles, then the single tab content can be named as RecentsList, DashboardList, etc.

Co-authored-by: Jesse Yang <jesse.yang@airbnb.com>

yardz · 2021-02-22T11:57:51Z

superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx

@@ -83,7 +83,7 @@ const ActivityTableRow: React.FC<ActivityProps> = ({
  <>
    {activityList.map(activity => (
      <CardStyles
-        key={`${activity.item_title}__${activity.time}`}
+        key={`${activity.dashboard_title}__${activity.time}__${activity.id}`}


This solved the problem of "not unique" keys.

@ktmud The "infinite cards bug" is a problem with keys (or lack thereof).

These types need a better definition. Several "mandatory" fields are present only sometimes (example, id) ...
This definition is not at all reliable...

If id is always present, then dashboard_title is probably not needed. Feel free to update the typing based on API responses.

@ktmud That's the problem, the id is not always present. On the main tab page it does not exist. In the other tabs it exists (with the test data). This typing is not reliable.

That's unfortunate and probably another reason why we want to do the typing refactoring first. It really isn't that hard to do because most of the types already exist somewhere else. See: #13291

ktmud · 2021-02-22T19:56:10Z

superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx

+import ListViewCard from 'src/components/ListViewCard';
+import { CardStyles } from '../utils';
+
+interface ActivityObjects {


We should probably have separate type for each entity, then use union type for "activity object".

type ActivityObject = RecentSlice | RecentDashboard

I agree, but again, this PR is not to improve the existing types but to solve a bug. Refactoring and any other type of improvement is not within the scope of this PR.

This one could be related since accurate typing may have an impact on how unique keys are chosen. Since this is not a user-facing bug, it might also make sense to do the type refactoring first (either in this or another PR) before merging this fix.

@ktmud Do you prefer a buggy code and a more complex code than a simpler and no bug code, because the type is not perfect?

Sorry it doesn't make any sense to me.

Please take a look at #13291

Adding typing won't necessary make the code more complex and it does help identify and simplify potential solutions (as demonstrated in my PR).

Current code wasn't buggy to the end users (on the key problem at least).

Adding typing won't necessary make the code more complex and it does help identify and simplify potential solutions (as demonstrated in my PR).

I didn't talk about the type but about the original component, which had 1 more component inside it. (2 components in 1).
This is a more complex component and much more difficult to test.

Current code (for React key at least) wasn't buggy to the end users.

If the user didn't see it then the bug doesn't exist?

Hey guys sorry i haven't been following the threads. based on the conversation above, my understanding is that
1)we are not fixing UI bug, meaning I wouldn't notice it as an end user. 2)there might be a more comprehensive solution to solve this issue.
@yardz I prioritize code quality + cleaning up technical debt in Q1-Q2, more than even fixing an actual UI bug. Let's make sure if refractor is necessary, we are not simply putting a bandaid on the code, but solve it from the root, also not get distracted by not prioritized tasks.
would you mind addressing @ktmud comment if that's not too much of work, or we can park this PR and move on.

cc @rusackas

If it does not impact the users, then there is no urgency to merge your fix. I personally feel typing is a bigger fish to catch than solving a developer warning that is not visible to users. You may disagree but there is no need to be sarcastic.

1)we are not fixing UI bug, meaning I wouldn't notice it as an end user.

This is not necessarily true. Just because our test data set is not generating a noticeable error does not mean that with a different data set this error is imperceptible. This key problem generates a lot of errors when a component is rerendered.

2)there might be a more comprehensive solution to solve this issue.

From what I saw, there are some requests that are not correctly typed (and are in js). A definitive and responsible solution would involve much more work and several PRs. I wouldn't risk putting a "definitive" type without having done this work before it...
I believe that this component needs some refinement "cycles" to achieve the "definitive" result

ktmud · 2021-02-23T08:57:56Z

Supersedes by #13291

bugfix: missing key on welcome page

e479456

pull-request-size bot added the size/L label Feb 18, 2021

junlincc added the home:others Related to Homepage / Welcome page label Feb 18, 2021

pkdotson approved these changes Feb 18, 2021

View reviewed changes

junlincc requested review from ktmud and rusackas February 20, 2021 01:41

ktmud reviewed Feb 22, 2021

View reviewed changes

yardz and others added 3 commits February 22, 2021 08:28

Update superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx

86d31c5

Co-authored-by: Jesse Yang <jesse.yang@airbnb.com>

fix: import order

7c4314f

fix: unique keys in the different tabs

470ef98

yardz commented Feb 22, 2021

View reviewed changes

yardz requested a review from ktmud February 22, 2021 18:48

ktmud reviewed Feb 22, 2021

View reviewed changes

ktmud mentioned this pull request Feb 23, 2021

fix(home): missing key and invalid dates in Recents cards #13291

Merged

6 tasks

ktmud closed this Feb 23, 2021

yardz deleted the bugfix/ActivityTable branch February 23, 2021 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: missing key on welcome page #13225

fix: missing key on welcome page #13225

yardz commented Feb 18, 2021

junlincc commented Feb 18, 2021

pkdotson Feb 18, 2021

yardz Feb 19, 2021

ktmud Feb 22, 2021 •

edited

Loading

yardz Feb 22, 2021 •

edited

Loading

ktmud Feb 22, 2021

ktmud left a comment

ktmud Feb 22, 2021 •

edited

Loading

yardz Feb 22, 2021 •

edited

Loading

yardz Feb 22, 2021

yardz Feb 22, 2021

ktmud Feb 22, 2021

yardz Feb 22, 2021

ktmud Feb 23, 2021

ktmud Feb 22, 2021

yardz Feb 22, 2021 •

edited

Loading

ktmud Feb 22, 2021

yardz Feb 22, 2021

ktmud Feb 23, 2021 •

edited

Loading

yardz Feb 23, 2021

junlincc Feb 23, 2021

ktmud Feb 23, 2021 •

edited

Loading

yardz Feb 23, 2021

ktmud commented Feb 23, 2021

fix: missing key on welcome page #13225

fix: missing key on welcome page #13225

Conversation

yardz commented Feb 18, 2021

SUMMARY

TEST PLAN

junlincc commented Feb 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

yardz Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud left a comment

Choose a reason for hiding this comment

ktmud Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

yardz Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yardz Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud commented Feb 23, 2021

ktmud Feb 22, 2021 •

edited

Loading

yardz Feb 22, 2021 •

edited

Loading

ktmud Feb 22, 2021 •

edited

Loading

yardz Feb 22, 2021 •

edited

Loading

yardz Feb 22, 2021 •

edited

Loading

ktmud Feb 23, 2021 •

edited

Loading

ktmud Feb 23, 2021 •

edited

Loading