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

"Stacked" Filter Issue on Satistics Page #995

Closed
SouthKoreaLN opened this issue Mar 29, 2024 · 11 comments · Fixed by #1138
Closed

"Stacked" Filter Issue on Satistics Page #995

SouthKoreaLN opened this issue Mar 29, 2024 · 11 comments · Fixed by #1138

Comments

@SouthKoreaLN
Copy link
Contributor

SouthKoreaLN commented Mar 29, 2024

Description

Based on the report by SN user Onions here.

On the satistics page, deselecting all filters, including the stacked filter, results in the display of data for the stacked type (see evidence bellow), contrary to the expected behavior of no data presentation.
Notably, accessing the satistics page directly from the link https://stacker.news/satistics?inc= correctly shows an empty result, indicating that the issue is in the behavior of selecting/deselecting the filters.

Steps to Reproduce

  1. Access the satistics page via the usual method.
  2. Deselect all filters, including the stacked filter.
  3. Observe the display of data for the stacked type.

Expected behavior

When all filters, including the stacked filter, are simultaneously deselected on the satistics page, no data should be displayed.

Screenshots

23787

Impact
This inconsistency in the display of data based on filter selection may lead to confusion among users and compromise the reliability of the satistics page.

@SouthKoreaLN
Copy link
Contributor Author

SouthKoreaLN commented Mar 29, 2024

FYI. I can reproduce this issue.

EDIT: It does remove some of the "stacked" entries when unticking the box, but not all.

@mateusdeap
Copy link

Me too. Looking at the console in my development environment, I noticed that I get this error:
image

I was unable to confirm wether it was related to this. Also, just looking visually, I have multiple entries for the same stacking event. Ticking the stacked box removes only one of them

@itsrealfake
Copy link
Contributor

itsrealfake commented May 1, 2024

image

React docs say don't generate the key while rendering the item.

The key is being generated while rendering here

{facts.map(f => <Fact key={f.type + f.id} fact={f} />)}

@itsrealfake
Copy link
Contributor

hmm... maybe we are getting funny data from this query and so Apollo isn't able to integrate data + previousData cache??

here's what i'm seeing:

first, i added a key String to the graphQL type & then requested it in query.
then i went to resolver/wallet.js and added a key to the Facts for testing...

image

now, i've i logged to the console from the render function and we can see in ssrData that there's a duplicated id (in this case it's 459394) between a stacked or spent item. interestingly, you can see that their random UUIDs are different

image image

@huumn
Copy link
Member

huumn commented May 1, 2024

I suspect this is an issue with apollo client cache, because this doesn't persist on refresh. It could be a key issue but I don't recall getting warnings about duplicates.

https://github.com/stackernews/stacker.news/blob/fd2008e5d1a2a8521a330a097b5aaf51bba6ce3d/lib/apollo.js#L207C1-L219C14

don't generate the key while rendering the item.

This doesn't mean they that they can't be constructed from deterministic values afaik. It just means for the same item in the list it needs to have the same key.

@itsrealfake
Copy link
Contributor

This doesn't mean they that they can't be constructed from deterministic values afaik.

I think you're right.

So i tried this:

From a cold boot of the app & hard refresh of the page: http://localhost:3000/satistics/history?inc=invoice,stacked,spent,withdrawal

I went to lib/apollo.js and added this console.log everything else on the branch is same as master:
image

I refreshed the page and was given 5 elements, 2 have matched "key":
image

image

I suspect this is an issue with apollo client cache

I think you're right. Also, I've only been able to get this duplicated key error in the inspector through messing with 'stacked' and 'spent'

I think Apollo uses the ID! value to handle caching. And i suspect the query is returning duplicates in the id value. Both 'stacked' and 'spent' are JOINing from "Item" and "ItemAct"

image

@huumn
Copy link
Member

huumn commented May 2, 2024

I think Apollo uses the ID! value to handle caching. And i suspect the query is returning duplicates in the id value. Both 'stacked' and 'spent' are JOINing from "Item" and "ItemAct"

Yep, this is the problem! That id needs to be unique for the apollo cache to work. We should be able to define a custom id function for it.

I think it's just adding a new type policy:

        Fact: {
          keyFields: ['id', 'type'],
        },

Untested but I reckon that's what we'd do.

itsrealfake added a commit to itsrealfake/stacker.news that referenced this issue May 2, 2024
closes stackernews#995

enables apollo cache to work for 'stacked' 'spent' in /statistics page.
itsrealfake added a commit to itsrealfake/stacker.news that referenced this issue May 2, 2024
closes stackernews#995

enables apollo cache to work for 'stacked' 'spent' in /statistics page.
@itsrealfake
Copy link
Contributor

That worked!

huumn pushed a commit that referenced this issue May 3, 2024
closes #995

enables apollo cache to work for 'stacked' 'spent' in /statistics page.
@huumn
Copy link
Member

huumn commented May 5, 2024

@mateusdeap do you have a lightning address I can send helpfulness sats to? If not can you send me a bolt11?

@mateusdeap
Copy link

mateusdeap commented May 13, 2024

I'm a bit new to this, would my lightning address on stacker news suffice? If so: mateusdeap@stacker.news

@huumn
Copy link
Member

huumn commented May 13, 2024

It should

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

Successfully merging a pull request may close this issue.

4 participants