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

Workspace improvements #178

Merged
merged 10 commits into from
Sep 6, 2024
Merged

Workspace improvements #178

merged 10 commits into from
Sep 6, 2024

Conversation

foysalit
Copy link
Contributor

@foysalit foysalit commented Sep 5, 2024

This PR adds a bunch of new improvements to workspace feature

  1. Take action in batches. If there are too many actions to be taken, we will now batch 50 actions at a time and then add a cooling period after each batch before firing the next one.
  2. Add all search results from repositories page to workspace. This allows mods to search for a user with a keyword and all matching users to workspace.
  3. Add all followers of a user to workspace.
    4 Add all follows of a user to workspace.

Copy link

render bot commented Sep 5, 2024

@foysalit foysalit marked this pull request as ready for review September 5, 2024 09:24
@foysalit foysalit requested review from matthieusieben and devinivy and removed request for matthieusieben September 5, 2024 09:24
Copy link
Contributor

@matthieusieben matthieusieben left a comment

Choose a reason for hiding this comment

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

Looks good.

useInfiniteQuery({
queryKey: ['followers', { id }],
queryFn: async ({ pageParam }) => {
const { data } = await labelerAgent.api.app.bsky.graph.getFollowers({
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: .api. no longer required (deprecated)

Suggested change
const { data } = await labelerAgent.api.app.bsky.graph.getFollowers({
const { data } = await labelerAgent.app.bsky.graph.getFollowers({

Comment on lines 47 to 48
// Only when pass the onClose here because if we pass it during instantiation, the update essentially triggers onClose
// causing the toast to close indefinitely
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't updating twice also cause this behavior ?

Would keeping another reference help ?

const toastNr = useRef<number>(0)
const toastId = useRef<number | string | null>(null)

// ...

onSuccess: () => {
  // ...

  const currentNr = ++toastNr.current
  const onClose = () => {
    // Only clear the toastId ref when the latest toast is closed
    if (toastNr.current === currentNr) toastId.current = null
  }

  // use onClose both for init and update
  // ...
},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks ended up using the isActive check.

)
.forEach((checkbox) => {
if (results.failed.includes(checkbox.value)) {
checkbox.checked = true
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm not mistaken, this could cause the react component's internal state not to be in sync with the actual checkbox state, leading to unexpected behavior. Indeed, setting checkbox.checked won't trigger the change listener that the react component typically listens for in order to keep the internal state in sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! the internal state is only kept for a UX detail which let's us do gmail style shift+click checkbox selection. which isn't that important to the functionalities. thanks for pointing this out, we're now triggering the mousedown event manually along with checked value update

@foysalit foysalit merged commit 864bfea into main Sep 6, 2024
3 checks passed
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.

2 participants