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

Refresh tags upon adding a puzzle #722

Closed
wants to merge 3 commits into from
Closed

Refresh tags upon adding a puzzle #722

wants to merge 3 commits into from

Conversation

kcaze
Copy link
Collaborator

@kcaze kcaze commented Dec 29, 2023

Address #672.

This re-fetches the hunt slice after adding a puzzle, so the new meta tag appears.

@rgossiaux
Copy link
Contributor

Does this fully fix the issue? I haven't tested it but from my understanding the tags are also missing when other people add metas (or if they create new tags in some other way)

I think we might want to just refresh the Hunt object periodically, like every 60s or so, similar to what we're doing with refreshing the puzzles. I think this change you're making to refresh it immediately after adding a puzzle is also good for the case when you add a meta and then immediately try to add a feeder puzzle to that meta, though we could check if it's a meta first as an optimization. For correctness we would also want to do an immediate refresh if you edit the puzzle to toggle whether it's a meta, I think.

@kcaze
Copy link
Collaborator Author

kcaze commented Dec 30, 2023

Yeah that makes sense. I'll add a periodic refresh and also a refresh when you edit tags.

@kcaze
Copy link
Collaborator Author

kcaze commented Jan 4, 2024

Tested editing a puzzle to change it to a meta and then change it to not a meta.

I opened two windows to test the periodic refresh for tags.

Copy link
Contributor

@rgossiaux rgossiaux left a comment

Choose a reason for hiding this comment

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

Actually stepping back for a second (sorry for the churn), does this change make sense to begin with? I kind of feel like what we should be doing is just storing the default tags on the Hunt object (which is fetched once when you load the page and is not expected to change), and then reading the list of available tags from the list of puzzles kind of like we did before:

https://github.com/cardinalitypuzzles/cardboard/pull/642/files#diff-2d036991b0fa7e0ecb01d9f9c592d66f59257862e92de3635f04f1555b19a000L231

But that logic can be simplified a lot now to just select all the tags from the Puzzle objects, combine with all the tags on the Hunt object, dedupe and sort by tag id.

Does that make sense?

@@ -26,6 +27,10 @@ function EditPuzzleModal({ huntId, puzzleId, name, url, isMeta, hasChannels }) {
},
})
).finally(() => {
// Used to update hunt tags.
dispatch(fetchHunt(huntId));
dispatch(fetchPuzzles(huntId));
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think this is necessary, is it?

@@ -23,6 +24,8 @@ function AddPuzzleModal({ huntId }) {
create_channels: createChannels,
})
).finally(() => {
// Used to update hunt tags.
dispatch(fetchHunt(huntId));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think only if isMeta is true do we need to do this, right?

@@ -26,6 +27,10 @@ function EditPuzzleModal({ huntId, puzzleId, name, url, isMeta, hasChannels }) {
},
})
).finally(() => {
// Used to update hunt tags.
dispatch(fetchHunt(huntId));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, we only need to do this if isMeta is different from newIsMeta right?

dispatch(fetchPuzzles(props.huntId));
dispatch(fetchHunt(props.huntId));
Copy link
Contributor

Choose a reason for hiding this comment

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

This might add a lot of load if we do it every 10 seconds

@kcaze
Copy link
Collaborator Author

kcaze commented Jan 9, 2024

Yeah that makes sense, I don't have a good sense of how much additional load fetching the hunt object is (since we already fetch puzzles every 10 seconds). From a perf standpoint, I guess ideally we have a websocket connection with which the server pushes updates?

It makes sense to me to merge the default tags from the hunt object + custom tags from the puzzles instead. I'll undo these changes and get tags that way.

@kcaze
Copy link
Collaborator Author

kcaze commented Jan 11, 2024

Closing in favor of #755.

@kcaze kcaze closed this Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants