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

[CLNP-6003] test: Add tests for hooks in src/hooks #1283

Open
wants to merge 27 commits into
base: feat/state-mgmt-migration-1
Choose a base branch
from

Conversation

git-babel
Copy link
Contributor

@git-babel git-babel commented Dec 10, 2024

Changelog

  • Added tests for hooks in src/hooks

before

스크린샷 2024-12-10 오후 4 09 19

after

스크린샷 2024-12-10 오후 4 08 14

AhyoungRyu and others added 23 commits December 3, 2024 21:03
## Overview
This PR refactors the MessageSearch state management system, introducing
a custom store solution and several hooks for improved performance,
maintainability, and type safety.

## New Files
- `src/utils/storeManager.ts`: Implements a custom store creation
utility
- `src/contexts/_MessageSearchContext.tsx`: Provides the MessageSearch
context and store \w new state mgmt logic. It's just temporal name.
- `src/hooks/useStore.ts`: A generic hook for accessing and updating
store state
- `src/hooks/useMessageSearchStore.ts`: A specialized hook for
MessageSearch state management
- `src/components/MessageSearchManager.tsx`: Manages MessageSearch state
and side effects
- `src/hooks/useMessageSearchActions.ts`: Manages action handlers 

## Updated Hooks
- `useSetChannel`: Now uses `useMessageSearchStore` directly
- `useSearchStringEffect`: Refactored to work with the new store
- `useGetSearchMessages`: Updated to utilize the new state management
system
- `useScrollCallback`: Adapted to work with the custom store

## Key Changes
1. Introduced a custom store solution to replace the previous
reducer-based state management.
2. Implemented `useStore` hook for type-safe and efficient state access
and updates.
3. Created `MessageSearchManager` to centralize state management logic.
4. Refactored existing hooks to work with the new store system.
5. Improved type safety throughout the MessageSearch module.
[CLNP-5044](https://sendbird.atlassian.net/browse/CLNP-5044)

### Fix
ChannelSettingsProvider migration
- Created a hook `useChannelSettings` for replacing
`useChannelSettingsContext`
- Created hooks `useSetChannel` and `useChannelHandler` to separate the
logics from the ChannelSettingsProvider
https://sendbird.atlassian.net/browse/CLNP-5043 
Two things are handled based on what I mentioned in
https://sendbird.atlassian.net/wiki/spaces/UIKitreact/pages/2511765635/UIKit+React+new+State+Management+Method+Proposal#4.1-Unit-Test
- [x] Added unit tests for `useMessageSearch` hook and new
`MessageSearchProvider`
- [x] Added integration tests for `MessageSearchUI` component

So the MessageSearch module test coverage has been changed 
**from** 
File --------------------------------------------------| % Stmts | %
Branch | % Funcs | % Lines | Uncovered Line #s
<img width="814" alt="Screenshot 2024-10-08 at 2 36 55 PM"
src="https://github.com/user-attachments/assets/c0fef6fe-0fc1-4f37-b74f-0486d70352a7">
**to** 
<img width="941" alt="after"
src="https://github.com/user-attachments/assets/7fc19fb8-810c-4256-8230-3884d11e109a">
note that it used to be like 0%, but now the test coverage of the newly
added files is almost 100%; green 🟩.



### Checklist

Put an `x` in the boxes that apply. You can also fill these out after
creating the PR. If unsure, ask the members.
This is a reminder of what we look for before merging your code.

- [x] **All tests pass locally with my changes**
- [x] **I have added tests that prove my fix is effective or that my
feature works**
- [ ] **Public components / utils / props are appropriately exported**
- [ ] I have added necessary documentation (if appropriate)
### Changelogs
This is a part of state management migration. This PR migrate
'GroupChannelListProvider' and related files to the new style.
* Created `useGroupChannelList()` hook. It'll replace the previous
`useGroupChannelContext()` hook.
fix: Prevent destroy error by adding `AbortController` in
`useSetChannel`
- Added `AbortController` to cancel async operations when the component
unmounts.
- Ensured that state updates are skipped if the operation is aborted.
- Prevented potential memory leaks and the `'destroy is not a function'`
error.
- Updated `useEffect` cleanup to properly handle pending async calls.

feat: Add tests for ChannelSettings migration
This PR contains the unit tests for the recent refactoring of
`GroupChannelListProvider`.
* add test about the new provider and its state management logic itself.
* add the integration test with `GroupChannelListUI` component.

---------

Co-authored-by: Irene Ryu <[email protected]>
…ern (#1246)

Addresses https://sendbird.atlassian.net/browse/CLNP-5047

This PR migrates the GroupChannelProvider to use a new state management
pattern. This change introduces a more predictable and maintainable way
to manage channel state while maintaining backward compatibility.

- Introduced new store-based state management
- Separated concerns between state management and event handling
- Added `useGroupChannel` hook for accessing state and actions
- Optimized performance with proper memoization

Old pattern:
```typescript
const { someState, someHandler } = useGroupChannelContext();
```
New pattern:
```typescript
// For state and actions
const { state, actions } = useGroupChannel();

// For handlers and props (backward compatibility)
const { someHandler } = useGroupChannelContext();
```

- More predictable state updates
- Better separation of concerns
- Enhanced performance through optimized renders

- All existing functionality remains unchanged
- Added tests for new hooks and state management
- Verified backward compatibility
- Tested with real-time updates and message handling

- [x] useGroupChannelContext is kept for backward compatibility
- [x] Unit & integration tests will be added for new hooks and state
management

Put an `x` in the boxes that apply. You can also fill these out after
creating the PR. If unsure, ask the members.
This is a reminder of what we look for before merging your code.

- [x] **All tests pass locally with my changes**
- [x] **I have added tests that prove my fix is effective or that my
feature works**
- [ ] **Public components / utils / props are appropriately exported**
- [ ] I have added necessary documentation (if appropriate)
#1250)

### Overview
This PR migrate ThreadProvider and related files to the new state
management pattern.

### Changelog
* `ThreadProvider` is migrated, and `useThread()` hook is introduced.
* Removed `ThreadDispatcher` usages in ThreadProvider; it is replaced
with the new state-action pattern of `useThread()`.
* `PubSub` of `config` still remains. It is out of scope of this PR.

### Remaining tasks
* Add unit tests and integration tests.

### FurtherConcern
* Handling hook
* The previous `ThreadProvider` contained several custom hooks. Those
hooks retrieved state and actions through `useThreadContext()`
* Due to that, replacing `useThreadContext()` to new `useThread()` faced
a problem. Those hooks �conatin `useThread()`, `useThread()` contains
the hooks. So it makes cycle.
* For now, I moved all functionality of the hooks to the `useThread()`,
but it looks wrong. Any good way to handle this?
…annelProvider (#1263)

Fixes 
- https://sendbird.atlassian.net/browse/CLNP-5917
- https://sendbird.atlassian.net/browse/CLNP-5918

### Changes 
To fix [CLNP-5917](https://sendbird.atlassian.net/browse/CLNP-5917)
introduced optimizations to prevent the "Maximum update depth exceeded"
error that occurs during message searches:

1. Added useDeepCompareEffect hook:
- Performs deep comparison of dependencies instead of reference equality
- Particularly useful for handling message array updates efficiently
- Inspired by
[kentcdodds/use-deep-compare-effect](https://github.com/kentcdodds/use-deep-compare-effect)

2. Enhanced useStore with state change detection:
- Added hasStateChanged helper to compare previous and next states
- Prevents unnecessary updates when state values haven't actually
changed
- Optimizes performance by reducing redundant renders

3. Improved storeManager with nested update prevention:
- Added protection against nested state updates
- Prevents infinite update cycles


Put an `x` in the boxes that apply. You can also fill these out after
creating the PR. If unsure, ask the members.
This is a reminder of what we look for before merging your code.

- [x] **All tests pass locally with my changes**
- [ ] **I have added tests that prove my fix is effective or that my
feature works**
- [ ] **Public components / utils / props are appropriately exported**
- [ ] I have added necessary documentation (if appropriate)


[CLNP-5917]:
https://sendbird.atlassian.net/browse/CLNP-5917?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
* Changelog
  * Fixed the broken unit tests of `GroupChannelListProvider`
  * Fixed the UI error when click `open in channel` in the other channel
* Use `useDeepCompareEffect` in `ThreadProvider` and
`GroupChannelListProvider`
)

Used `useDeepCompareEffect` for the `updateState` of `SendbirdProvider`

Co-authored-by: Irene Ryu <[email protected]>
- before
<img width="1099" alt="Screenshot 2024-12-03 at 11 14 47 PM"
src="https://github.com/user-attachments/assets/75716138-2ac6-46d1-8fe7-d3793d53c3be">

- after
<img width="1107" alt="Screenshot 2024-12-03 at 11 13 43 PM"
src="https://github.com/user-attachments/assets/182237d5-c322-4d59-8595-3abdaf15afa9">

GroupChannelProvider has some not covered lines but they're mostly from
uikit-tools lib related logic. There could be a way to mock these lines
too, but let me handle it separately.
…ider (#1272)

Fixes 
- https://sendbird.atlassian.net/browse/CLNP-5966
- https://sendbird.atlassian.net/browse/CLNP-5967
- https://sendbird.atlassian.net/browse/CLNP-5969
- https://sendbird.atlassian.net/browse/CLNP-5971
- https://sendbird.atlassian.net/browse/CLNP-5973

## When to use useDeepCompareEffect vs useEffect

### useDeepCompareEffect is useful when:

1. **Handling objects without guaranteed immutability**
```typescript
const complexObject = {
  settings: {
    theme: { ... },
    preferences: { ... }
  },
  data: [ ... ]
};

useDeepCompareEffect(() => {
  // When you want to detect actual value changes
}, [complexObject]);
```


2. **Working with data from external libraries or APIs**
- When objects have new references but identical content

3. **Dealing with deeply nested objects where memoization is
impractical**
- When object structures are too complex for individual memoization

### useEffect is better when:

1. **Detecting changes in array items**

```typescript
const items = [{id: 1, value: 'a'}, {id: 2, value: 'b'}];
// Better for detecting changes within array items
useEffect(() => {
  // Detect changes in items array
}, [items]);
```


2. **Performance is critical**
- Deep comparison is computationally expensive
- Especially important for large arrays or frequently updating data

### Example of proper useDeepCompareEffect usage:
```typescript
useDeepCompareEffect(() => {
  updateState({
    ...configurations,
    ...scrollState,
    ...eventHandlers,
  });
}, [
  configurations,
  scrollState,
  eventHandlers,
]);
```

This works well here because:
- Dependencies are mostly objects
- Updates are needed only when internal structure changes
- Objects are already memoized, reducing deep comparison cost

### Key Takeaway:
- Use useDeepCompareEffect when structural equality matters
- Use useEffect for reference equality or primitive value changes
- Consider the trade-off between performance and accuracy

---------

Co-authored-by: junyoung.lim <[email protected]>
Fixes https://sendbird.atlassian.net/browse/CLNP-5981 and applied the
same approach in
#1272

### Checklist

Put an `x` in the boxes that apply. You can also fill these out after
creating the PR. If unsure, ask the members.
This is a reminder of what we look for before merging your code.

- [x] **All tests pass locally with my changes**
- [ ] **I have added tests that prove my fix is effective or that my
feature works**
- [ ] **Public components / utils / props are appropriately exported**
- [ ] I have added necessary documentation (if appropriate)
[CLNP-5737](https://sendbird.atlassian.net/browse/CLNP-5737)

* Added tests for `Sendbird/index.tsx`, `Sendbird/utils.ts`,
`SendbirdProvider.tsx`, `initialState.ts`, and `useSendbird.tsx`

### Before

![image](https://github.com/user-attachments/assets/0bdae22e-f5f5-4880-949c-33ea65d61a29)

### After

![image](https://github.com/user-attachments/assets/b0f6d021-bb80-49cd-b476-cc5a6c155c3e)
### Changelog
This PR will add the test cases for the new `ThreadProvider` and its
related hooks.
#### Before
<img width="1163" alt="스크린샷 2024-12-09 오전 11 29 53"
src="https://github.com/user-attachments/assets/40ffc29e-0774-4ac7-973d-15af55bc88fd">

#### After
<img width="1199" alt="스크린샷 2024-12-09 오전 11 28 39"
src="https://github.com/user-attachments/assets/ea3366c0-2f11-4e1f-af6f-a423d006ab42">

---------

Co-authored-by: Irene Ryu <[email protected]>
…ok (#1278)

#### Before
<img width="705" alt="Screenshot 2024-12-06 at 6 23 24 PM"
src="https://github.com/user-attachments/assets/8269506e-b347-4091-a3d7-254aabb063d9">

#### After
<img width="703" alt="Screenshot 2024-12-06 at 6 36 19 PM"
src="https://github.com/user-attachments/assets/4480241a-4bfd-4096-a13f-27f7c6ac76ba">
Copy link

netlify bot commented Dec 10, 2024

Deploy Preview for sendbird-uikit-react ready!

Name Link
🔨 Latest commit e36e235
🔍 Latest deploy log https://app.netlify.com/sites/sendbird-uikit-react/deploys/675926d20bb3e500084f52be
😎 Deploy Preview https://deploy-preview-1283--sendbird-uikit-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

git-babel and others added 2 commits December 10, 2024 16:25
)

Fixes https://sendbird.atlassian.net/browse/CLNP-6022

This PR addresses an issue where the scroll position was not correctly
set to the bottom when switching from a channel with few messages to one
with many messages.

The problem was resolved by adding a delay to ensure the scroll
reference is updated before attempting to scroll to the bottom. This
change ensures that users always see the latest messages when switching
channels.
@AhyoungRyu AhyoungRyu changed the title feat: Add tests for hooks in src/hooks [CLNP-6003] test: Add tests for hooks in src/hooks Dec 10, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems nowhere is using this hook. So I think we can remove the file 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the file.

@AhyoungRyu AhyoungRyu force-pushed the feat/state-mgmt-migration-1 branch from 68e0cb8 to 21a625d Compare December 11, 2024 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants