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

fix: timetable selection 페이지 에러 처리 #82

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

owl1753
Copy link
Contributor

@owl1753 owl1753 commented Feb 14, 2025

#️⃣연관된 이슈

📝작업 내용

  • ErrorBoundary를 Sentry.ErrorBoundary로 변경했어요.
  • useTimetables 훅을 만들었어요.
  • TimetableSelection을 ErrorBoundary와 Suspense로 감싸서 fallback을 보여줘요.

💬리뷰 요구사항(선택)

  • useTimetables에서 문제될 부분이 있나요?

@owl1753 owl1753 requested a review from 2wndrhs February 14, 2025 06:56
@owl1753 owl1753 self-assigned this Feb 14, 2025
@owl1753 owl1753 requested a review from fecapark February 14, 2025 07:00
@owl1753
Copy link
Contributor Author

owl1753 commented Feb 14, 2025

페카가 슬랙 쓰레드에 에러 처리 관심있다 하셔서 리뷰 넣어드렸어요

@2wndrhs
Copy link
Member

2wndrhs commented Feb 15, 2025

https://yourssu.slack.com/archives/C8UT5E95E/p1739554674792619 스레드에서 논의된 것처럼 추천 시간표 조회 API 요청이 분리될 예정이라 일단 ff9ab0b 커밋까지만 머지되면 좋을 것 같아요

이후 변경된 추천 시간표 조회 API 연결과 시간표 선택 페이지에서의 에러 처리는 제가 진행해보고 싶어요!

+) ky의 timeout 옵션을 false로 하는 것보다 5초? 정도로 충분히 긴 시간을 주는 것이 좋지 않을까요
timeout 옵션을 false로 주면 서버의 응답이 없는 상황에서 사용자가 계속 Skeleton만 보게 하는 것보다는 에러를 발생시킨 다음 사용자로 하여금 다시 API 요청을 시도하게끔 하는게 좋을 것 같아서요

@owl1753
Copy link
Contributor Author

owl1753 commented Feb 15, 2025

ky에서 기본적으로 설정되어 있는 timeout이 10초인데, 서버가 과부하되면 10초보다 더 걸리는 경우가 생겨서 time을 없에놨어요. 특히 시간표 생성의 경우는 알고리즘의 시간 복잡도 상 10초보다 더 걸리는 경우가 있기 때문에 시간을 설정할거면 아예 30초? 정도로 늘리는게 나을 거 같아요.

@owl1753
Copy link
Contributor Author

owl1753 commented Feb 15, 2025

그리고 백엔드 스펙이 바뀐다 하더라도 기존 status를 이용한 방식에서 ErrorBoundary와 Suspense를 이용한 방식으로 코드가 리팩토링 되는 건 마찬가지이기 때문에 지금 상태로 머지해도 큰 상관은 없을 거 같아요.

@2wndrhs
Copy link
Member

2wndrhs commented Feb 15, 2025

추천 시간표 조회 API가 polling 하면서 시간표 생성이 되었는지 확인하는 방식으로 변경되어서 지금의 Promise를 던져 Suspense를 일으키는 방법으로는 정상적으로 Skeleton UI가 표시가 안될 것 같아요 (Mutation 대신 Query를 사용해야 되기도 하고)

polling 방식으로 변경되면서 Query 옵션 중 refetchInterval을 건드려야 할 것 같은데 SuspenseQuery와 refetchInterval을 같이 사용하면 처음 추천 시간표 조회 API 요청 시에만 Suspense Fallback이 표시되는 문제도 있어서 이후 쿼리들은 isFetching 값을 검사해서 Fallback을 표시해줘야 될 것 같네요

시간표 선택 페이지를 제가 만들기도 했고 맥락을 더 잘 아는 제가 수정하는게 좋을 것 같아 말씀드렸습니다.

@owl1753
Copy link
Contributor Author

owl1753 commented Feb 15, 2025

지금 TimetableSelection에서 사용하는 mutation의 동작과는 상관없이 useMutationState를 useSuspenseQuery처럼 사용할 수 있도록 커스텀한 것에 가깝기 때문에 Query로 바뀌더라도 큰 구조상의 변화는 없을 거 같아요.

polling으로 스펙이 바뀌게 되면 useTimetables 훅에서 isFetching 값을 검사해서 Promise를 던져도 되고(안해봐서 될지는 모르겠지만), 아니면 Suspense를 안쓰고 TimetableSelection Component에서 isFetching값에 따라 Fallback을 내려줘도 되겠죠.

결국 에러는 ErrorBoundary에서 처리해줘야하기 때문에 컴포넌트를 분리해야 하는 것은 필수적이라고 생각해요.

말씀하신대로 추후 수정 작업은 시간표 선택 페이지 맥락을 더 잘 아는 준이 이어서 하는게 좋을 것 같아요!

@2wndrhs
Copy link
Member

2wndrhs commented Feb 15, 2025

음.. 저번에 슬렉에서 말했던 것처럼 Promise를 던져 Suspense를 일으키는 것이 React에서 권장하지 않는 방법이기도 하고 예상치 못한 문제들이 발생할 때 대응이 어려울 것 같아서 useTimetables()처럼 React Query에 추상화 레벨을 더한 훅을 사용하는 것은 원하지 않는 방향이긴 합니다.

또한 기존 TimetableSelectionActivity 컴포넌트를 TimetableSelection, TimetableSelectionView, TimetableSelectionFallback 컴포넌트로 분리하여 TimetableSelectionView외부에서 props를 주입 받아 Fallback과 정상적인 데이터 상태를 모두 보여주는 것도 데이터 흐름을 파악하기 더 복잡해진 것 같아요 (TimetableSelectionView의 요구사항이 변경되면 props는 계속 추가되어야 하는 걸까요?)

저는 성급한 추상화는 지양하는 편이 좋다고 생각하고 진정 추상화의 필요성을 느낄 때 적용하는 것이 좋다고 생각합니다.

aha-programming
The-Wet-Codebase

@owl1753
Copy link
Contributor Author

owl1753 commented Feb 15, 2025

TimetableSelectionTimetableFallback으로 구분한 이유는 TimetableSelection에서는 useMutationState를 사용해서 에러와 Promise를 던지게 되는데 ErrorBoundary와 Suspense에 Fallback UI를 전달 할때는 useMutationState를 사용하지 않는 컴포넌트를 전달해야 하기 때문이에요. 만약 TimetableSelection을 분리하지 않는다면 ErrorBoundary와 Suspense가 렌더링되면서 불필요한 데이터 로딩이 발생하겠죠.

직접 Promise를 던지기보다 react19에서 권장하는 use 훅을 사용하는 것도 좋은 방법이라고 생각해요.

@2wndrhs
Copy link
Member

2wndrhs commented Feb 15, 2025

TimetableSelectionTimetableFallback으로 구분한 이유는 이해가 됐습니다.

하지만 TimetableSelectionView를 별도의 컴포넌트로 분리하여 props를 주입 받아 pending, success, error 상태에 대한 컴포넌트를 보여주는 것은 여전히 데이터 흐름이 복잡하고 변경에 유연하지 못하다고 생각합니다. (에러 페이지에서 에러 메시지와 재요청 버튼을 보여주는 대신 Dialog를 띄워주고 해당 Dialog에서 재요청 버튼을 누르는 방식으로 요구사항이 변경된다면 어떻게 해야될까요?)

TimetableSelectionView 컴포넌트를 별도로 분리하지 말고 TimetableSelection 컴포넌트와 합쳐 TimetableSelection 컴포넌트에서는 API 요청이 성공한 상태에 집중하게 하고 TimetableSelectionFallback 컴포넌트를 TimetableSelectionSuspenseFallback, TimetableSelectionErrorFallback 컴포넌트로 나누어 API 요청의 pending, error 상태에 대한 책임을 각 컴포넌트가 담당하게 하면 어떨까요?

@owl1753
Copy link
Contributor Author

owl1753 commented Feb 15, 2025

SuspenseFallbackErrorFallback 그리고 TimeSelection 의 UI 구조가 동일하기 때문에 UI의 재사용성을 높이기 위해서 View 컴포넌트에 UI 구조를 정의하고 데이터를 props로 받아온 것인데, 저는 개인적으로 변경 유연성보다는 재사용성이 좀 더 중요한 것 같아요. 물론 페이지에 새로운 정보를 추가하려면 수정해야 하는 파일 수가 많아지므로 확장성이 떨어진다는 것에는 동의해요.

데이터 흐름은 상위 컴포넌트에서 하위 컴포넌트로 흐르는 단방향 흐름이라고 생각했는데, 혹시 어떤 부분이 복잡하다고 생각하시는지 알려주실 수 있을까요?

@2wndrhs
Copy link
Member

2wndrhs commented Feb 15, 2025

TimetableSelectionFallback 컴포넌트에서 pendingerror 상태에 따른 컴포넌트를 모두 관리하는 부분과 TimetableSelectionView에서 많은 props들을 받아 UI를 표시해주는게 복잡하다고 느껴졌습니다.

props를 줄이면서 UI 재사용성을 높이려면 합성 컴포넌트 패턴을 사용해도 좋을 것 같은데 이부분은 저도 고민해보겠습니다.

@owl1753
Copy link
Contributor Author

owl1753 commented Feb 15, 2025

그러면 Fallback 컴포넌트는 SuspenseFallback과 ErrorFallback으로 구분해서 관리하고 View의 props를 줄이는 방향으로 고려해봅시당

@2wndrhs
Copy link
Member

2wndrhs commented Feb 15, 2025

합성 컴포넌트 패턴으로 리팩토링하면 아래 코드처럼 작성할 수 있을 것 같아요

  • TimetableSelection.tsx
const TimetableSelectionRoot = ({ children }: PropsWithChildren) => {
  return (
    <motion.div
      initial={{ y: 20, opacity: 0 }}
      animate={{ y: 0, opacity: 1 }}
      transition={{ duration: 0.4, ease: 'easeOut' }}
      className="mt-6 flex flex-1 flex-col items-center"
    >
      {children}
    </motion.div>
  );
};

const TimetableSelectionTitle = ({ children }: PropsWithChildren) => {
  return <h2 className="text-center text-[28px] font-semibold whitespace-pre-wrap">{children}</h2>;
};

const TimetableSelectionContent = ({ children }: PropsWithChildren) => {
  return <div className="mt-4 flex w-full flex-1 flex-col px-10 pb-4">{children}</div>;
};

const TimetableSelectionButton = ({
  children,
  className,
  ...props
}: ButtonHTMLAttributes<HTMLButtonElement>) => {
  return (
    <div className="sticky bottom-6 flex w-full justify-center">
      <button
        type="button"
        className={twMerge(
          'bg-primary w-50 rounded-2xl py-3.5 font-semibold text-white shadow-sm',
          className,
        )}
        {...props}
      >
        {children}
      </button>
    </div>
  );
};

const TimetableSelection = () => {
  const [selectedIndex, setSelectedIndex] = useState(0);
  const timetables = useTimetables();

  const { push } = useFlow();

  const timetableRefs = useRef<(HTMLDivElement | null)[]>([]);

  const handleTimetableClick = (index: number) => {
    setSelectedIndex(index);
    timetableRefs.current[index]?.scrollIntoView({
      behavior: 'smooth',
      block: 'center',
    });
  };

  const handleNextClick = () => {
    const selectedTimetable = timetables[selectedIndex];
    const unSelectedTimetable = timetables.filter(
      (timetable) => timetable.timetableId !== selectedTimetable.timetableId,
    );

    // Mixpanel 이벤트 추적
    Mixpanel.trackTimetableSelectionClick(selectedTimetable, unSelectedTimetable);

    push('TimetableSharingActivity', {
      timetableId: selectedTimetable.timetableId,
    });
  };

  return (
    <TimetableSelection.Root>
      <TimetableSelection.Title>{'사용자님을 위한\n시간표를 가져왔어요!'}</TimetableSelection.Title>

      <TimetableSelection.Content>
        {timetables.map((timetable, index) => (
          <div
            key={timetable.timetableId}
            className="pt-4 first:pt-0"
            data-index={index}
            ref={(element) => {
              {
                /* div 요소가 마운트 될 때 실행*/
              }
              timetableRefs.current[index] = element;
            }}
            onClick={() => handleTimetableClick(index)}
          >
            <Timetable
              timetable={timetable}
              className={`${
                index === selectedIndex ? 'border-primary' : 'border-placeholder'
              } transition-colors duration-300`}
            >
              <Timetable.Header
                className={`${
                  index === selectedIndex
                    ? 'bg-primary text-white'
                    : 'border-placeholder border-b-1'
                } transition-colors duration-300`}
              />
            </Timetable>
          </div>
        ))}
      </TimetableSelection.Content>
      <TimetableSelection.Button onClick={handleNextClick}>
        이 시간표가 좋아요
      </TimetableSelection.Button>
    </TimetableSelection.Root>
  );
};

TimetableSelection.Root = TimetableSelectionRoot;
TimetableSelection.Title = TimetableSelectionTitle;
TimetableSelection.Content = TimetableSelectionContent;
TimetableSelection.Button = TimetableSelectionButton;

export default TimetableSelection;
  • TimetableSelectionSuspenseFallback.tsx
const TimetableSelectionSuspenseFallback = () => {
  return (
    <TimetableSelection.Root>
      <TimetableSelection.Title>
        {'사용자님을 위한\n시간표를 가져오는 중이에요!'}
      </TimetableSelection.Title>

      <TimetableSelection.Content>
        <TimetableSkeleton className="pt-4">
          <TimetableSkeleton.Header />
        </TimetableSkeleton>
      </TimetableSelection.Content>

      <TimetableSelection.Button disabled={true} className="bg-gray-300">
        이 시간표가 좋아요
      </TimetableSelection.Button>
    </TimetableSelection.Root>
  );
};

export default TimetableSelectionSuspenseFallback;
  • TimetableSelectionErrorFallback.tsx
const TimetableSelectionErrorFallback = () => {
  const { pop } = useFlow();

  const handleClickButton = () => {
    pop(2);
  };

  return (
    <TimetableSelection.Root>
      <TimetableSelection.Title>
        {'사용자님을 위한\n시간표를 찾지 못했어요..'}
      </TimetableSelection.Title>

      <TimetableSelection.Content>
        <div className="flex flex-1 flex-col items-center justify-center">
          <img src={Warning} alt="Warning" className="size-42.5" />
        </div>
      </TimetableSelection.Content>

      <TimetableSelection.Button onClick={handleClickButton}>다시 만들기</TimetableSelection.Button>
    </TimetableSelection.Root>
  );
};

export default TimetableSelectionErrorFallback;
  • TimetableSelectionActivity.tsx
const TimetableSelectionActivity: ActivityComponentType = () => {
  return (
    <AppScreen>
      <div className="flex min-h-dvh flex-col py-6">
        <AppBar progress={100} />
        <SoongptErrorBoundary clientErrorComponent={<TimetableSelectionErrorFallback />}>
          <Suspense fallback={<TimetableSelectionSuspenseFallback />}>
            <TimetableSelection />
          </Suspense>
        </SoongptErrorBoundary>
      </div>
    </AppScreen>
  );
};

export default TimetableSelectionActivity;

@owl1753
Copy link
Contributor Author

owl1753 commented Feb 15, 2025

합성 컴포넌트 한 번도 안써봤는데 코드가 훨씬 깔끔해지는군요 ㄷㄷ

이거 참고해서 리팩토링하겠습니다!

@2wndrhs
Copy link
Member

2wndrhs commented Feb 15, 2025

  • useTimetable.ts
    case 'pending':
      throw new Promise<void>((resolve) => {
        requestAnimationFrame(() => {
          resolve();
        });
      });

여기서 requestAnimationFrame()을 사용하는 이유는 무엇인가요?
바로 resolve()를 호출하면 안되나요?

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