feat: 커뮤니티 기능 재활성화 #88
Conversation
WalkthroughThe pull request introduces significant modifications to the navigation and error handling logic across various components in the application. Key changes include the removal of the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (10)
src/utils/globalNavigate.ts (1)
1-1: Consider using a more robust state management solutionUsing a mutable global variable for navigation state could lead to race conditions and make the code harder to test.
Consider using React Context or a state management library like Redux/Zustand instead:
import { createContext, useContext } from 'react'; import { NavigateFunction } from 'react-router-dom'; const NavigateContext = createContext<NavigateFunction | null>(null); export const NavigateProvider = NavigateContext.Provider; export const useGlobalNavigate = () => { const navigate = useContext(NavigateContext); if (!navigate) throw new Error('NavigateProvider not found'); return navigate; };src/components/layout/index.tsx (2)
15-18: Optimize scroll event listener with debouncingThe scroll event can fire frequently and impact performance.
Consider implementing debouncing:
+import { debounce } from 'lodash'; + useEffect(() => { - window.addEventListener("scroll", handleScroll); - return () => window.removeEventListener("scroll", handleScroll); + const debouncedHandleScroll = debounce(handleScroll, 150); + window.addEventListener("scroll", debouncedHandleScroll); + return () => { + debouncedHandleScroll.cancel(); + window.removeEventListener("scroll", debouncedHandleScroll); + }; }, []);
Line range hint
27-33: Translate Korean comments to English for consistencyConsider translating the Korean comment for better maintainability in an international team context.
- // 사용자의 scroll 동작 여부에 따라 상태 값 변경 + // Update state based on user scroll positionsrc/api/index.ts (1)
33-34: Consider using type-safe localStorage operationsWhile the logic is correct, consider using a type-safe wrapper for localStorage operations to prevent potential runtime errors.
- localStorage.removeItem("FLOWBIT_ACT"); + const FLOWBIT_ACT = "FLOWBIT_ACT" as const; + localStorage.removeItem(FLOWBIT_ACT);src/app/signin/index.tsx (1)
31-36: Add missing dependency to useEffectThe effect uses
setLoginbut doesn't include it in the dependency array. WhilesetLoginis stable (from jotai), it's best practice to include all dependencies.- }, []); + }, [setLogin]);src/components/layout/header.tsx (2)
35-52: Add type definitions for modal parametersConsider defining an interface for the modal parameters to improve type safety and maintainability.
+interface ModalParams { + title: string; + content: string; + callBack: () => void; +} const validateLoginState = () => { if (isLogin) { navigate(COMMUNITY_URL); } else { - open({ + open<ModalParams>({ title: "로그인을 먼저 진행해주세요", content: "플로우빗 커뮤니티 기능은 회원을 위한 기능입니다.\n원활한 서비스 이용을 위해 로그인을 먼저 진행해주세요", callBack: () => { close(); navigate("/signin"); }, }); } };
223-298: Extract styles and add loading state for logoutConsider these improvements:
- Extract repeated styles to reusable constants
- Add loading state for logout operation
- Consider moving user options to a separate component for better maintainability
Example of extracting styles:
+const userOptionStyles = css` + ${DESIGN_SYSTEM_TEXT.CAPTION} + color: ${(props) => props.isScroll ? DESIGN_SYSTEM_COLOR.GRAY_800 : "white"}; + font-weight: 300; +`; - css={css` - ${DESIGN_SYSTEM_TEXT.CAPTION} - color: ${isScroll ? DESIGN_SYSTEM_COLOR.GRAY_800 : "white"}; - font-weight: 300; - `} + css={userOptionStyles}src/components/app/community/community-board.tsx (1)
390-401: Consider enhancing comment validationWhile the length check is good, consider trimming whitespace and preventing empty comments.
-if (comment.length > 0) { +const trimmedComment = comment.trim(); +if (trimmedComment.length > 0) { postComment({ boardId: props.boardId, - content: comment, + content: trimmedComment, }); }src/app/home/index.tsx (2)
94-111: Consider improving modal content accessibilityWhile the login validation logic is correct, the modal content could be more accessible.
content: - "플로우빗 커뮤니티 기능은 회원을 위한 기능입니다.\n원활한 서비스 이용을 위해 로그인을 먼저 진행해주세요", + "플로우빗 커뮤니티 기능은 회원을 위한 기능입니다.\n\n원활한 서비스 이용을 위해 로그인을 먼저 진행해주세요.\n\n계속하시겠습니까?",
45-45: Fix eslint warning about unused variableThe underscore variable is unused.
-const [isLogin, _] = useAtom(loginState); +const [isLogin] = useAtom(loginState);🧰 Tools
🪛 eslint
[error] 45-45: '_' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
src/assets/lock.svgis excluded by!**/*.svgsrc/assets/mail.svgis excluded by!**/*.svg
📒 Files selected for processing (16)
src/api/index.ts(2 hunks)src/app/community/index.tsx(1 hunks)src/app/home/index.tsx(10 hunks)src/app/predict/index.tsx(1 hunks)src/app/signin/index.tsx(3 hunks)src/components/app/community/community-board.tsx(8 hunks)src/components/app/community/community-tab.tsx(1 hunks)src/components/common/modal/Modal.tsx(1 hunks)src/components/layout/header.tsx(4 hunks)src/components/layout/index.tsx(1 hunks)src/components/layout/sidebar.tsx(2 hunks)src/hooks/api/community/UseApiComminity.ts(3 hunks)src/hooks/api/member/useApiSignIn.ts(1 hunks)src/hooks/api/member/useApiSignUp.ts(0 hunks)src/utils/common.ts(1 hunks)src/utils/globalNavigate.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/hooks/api/member/useApiSignUp.ts
✅ Files skipped from review due to trivial changes (3)
- src/utils/common.ts
- src/components/app/community/community-tab.tsx
- src/app/predict/index.tsx
🧰 Additional context used
🪛 eslint
src/app/home/index.tsx
[error] 45-45: '_' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🪛 Biome (1.9.4)
src/components/app/community/community-board.tsx
[error] 235-236: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (8)
src/components/layout/index.tsx (1)
22-24: LGTM! Proper useEffect dependency handling
The useEffect correctly includes navigate in its dependency array, ensuring the global navigation stays in sync with any navigation function updates.
src/hooks/api/community/UseApiComminity.ts (1)
21-21: LGTM! Clean refactor of exports
The consolidation of exports at the end of the file improves code organization while maintaining the same functionality. This follows the common pattern of grouping exports together.
Also applies to: 43-43, 70-71
src/components/layout/sidebar.tsx (1)
118-129: LGTM! Consistent styling for mypage section
The mypage section styling follows the design system correctly using DESIGN_SYSTEM_TEXT and DESIGN_SYSTEM_COLOR constants.
src/components/common/modal/Modal.tsx (2)
105-114: LGTM! Clean implementation of modal buttons
The cancel and confirm buttons are properly implemented with consistent styling using the design system.
105-114: Verify login notification modal content
The PR mentions adding login notifications, but the modal content for incorrect login attempts isn't visible in this change.
src/app/community/index.tsx (1)
199-199: LGTM: Enhanced null safety with optional chaining
The addition of optional chaining operators (?.) provides better protection against undefined values in the data object.
src/components/app/community/community-board.tsx (1)
257-266: LGTM: Well-implemented like functionality
The like button handler correctly manages state and handles both like and unlike scenarios.
src/app/home/index.tsx (1)
475-475: LGTM: Proper login validation before community access
The click handler correctly uses validateLoginState to ensure users are logged in before accessing community features.
| const signIn = ({ email, password }: signInProps) => { | ||
| const res = api.post(`/user-service/api/v1/member/login`, { | ||
| userId: email, | ||
| password: password, | ||
| }); | ||
| return res; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve type safety
The signIn function should include proper error handling and type definitions for the response.
Consider applying these improvements:
-const signIn = ({ email, password }: signInProps) => {
+interface SignInResponse {
+ accessToken: string;
+ // add other expected response fields
+}
+
+const signIn = async ({ email, password }: signInProps): Promise<SignInResponse> => {
+ try {
const res = api.post(`/user-service/api/v1/member/login`, {
userId: email,
password: password,
});
- return res;
+ return (await res).data;
+ } catch (error) {
+ throw new Error('Failed to sign in: ' + (error instanceof Error ? error.message : 'Unknown error'));
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const signIn = ({ email, password }: signInProps) => { | |
| const res = api.post(`/user-service/api/v1/member/login`, { | |
| userId: email, | |
| password: password, | |
| }); | |
| return res; | |
| }; | |
| interface SignInResponse { | |
| accessToken: string; | |
| // add other expected response fields | |
| } | |
| const signIn = async ({ email, password }: signInProps): Promise<SignInResponse> => { | |
| try { | |
| const res = api.post(`/user-service/api/v1/member/login`, { | |
| userId: email, | |
| password: password, | |
| }); | |
| return (await res).data; | |
| } catch (error) { | |
| throw new Error('Failed to sign in: ' + (error instanceof Error ? error.message : 'Unknown error')); | |
| } | |
| }; |
| export const navigateTo = (path: string) => { | ||
| if (globalNavigate) { | ||
| globalNavigate(path); | ||
| } else { | ||
| console.error("navigateTo was called before navigate was initialized"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and type safety for navigation
The current implementation uses console.error which might not be sufficient for production environments.
Consider these improvements:
+type NavigationPath = '/signin' | '/community' | '/home'; // add all valid paths
+
-export const navigateTo = (path: string) => {
+export const navigateTo = (path: NavigationPath) => {
if (globalNavigate) {
globalNavigate(path);
} else {
- console.error("navigateTo was called before navigate was initialized");
+ // Consider using a proper error tracking service
+ const error = new Error("Navigation attempted before initialization");
+ // reportError(error); // Use your error tracking service
+ throw error;
}
};Committable suggestion skipped: line range outside the PR's diff.
| <NavLink to={COMMUNITY_URL}> | ||
| <li onClick={setIsOpen}>커뮤니티</li> | ||
| </NavLink> |
There was a problem hiding this comment.
💡 Codebase verification
Login verification is missing in sidebar navigation
The codebase shows that login verification is properly implemented in both header.tsx and home/index.tsx using the validateLoginState function, which checks isLogin before allowing community access. However, the sidebar implementation allows direct navigation without this check, creating an inconsistency in authentication logic.
src/components/layout/sidebar.tsx: Add login verification to the community link using the existingvalidateLoginStatepattern
<NavLink to={COMMUNITY_URL}>
<li onClick={setIsOpen}>커뮤니티</li>
</NavLink>🔗 Analysis chain
Verify login check implementation for community access
According to the PR objectives, there should be login verification before accessing the community feature. However, the current implementation allows direct navigation without checking login status.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for login verification implementation
rg -l "validateLoginState.*community"
Length of output: 39
Script:
#!/bin/bash
# Let's search for any login-related checks or authentication guards for community routes
rg -A 3 "community.*route|route.*community|COMMUNITY_URL"
# Also search for any authentication-related hooks or components
rg -l "useAuth|isAuthenticated|requireAuth|PrivateRoute|AuthGuard"
Length of output: 1849
Script:
#!/bin/bash
# Let's check the validateLoginState function implementation since we see it's used in header.tsx
ast-grep --pattern 'function validateLoginState' -A 10
# Also check for any other login validation functions
rg -A 5 "validateLoginState|checkLogin|isLoggedIn"
Length of output: 1462
| alert("에러가 발생했습니다."); | ||
| }); | ||
| }} | ||
| onClick={handleSignIn} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Disable button during sign-in process
The button should be disabled while the sign-in request is in progress to prevent multiple submissions.
- onClick={handleSignIn}
+ disabled={isLoading}
+ onClick={handleSignIn}Committable suggestion skipped: line range outside the PR's diff.
| const handleSignIn = async () => { | ||
| signIn({ email, password }) | ||
| .then((res) => { | ||
| localStorage.setItem("FLOWBIT_ACT", res.data.accessToken); | ||
| setLogin(true); | ||
| navigate("/community"); | ||
| }) | ||
| .catch(({ status }) => { | ||
| if (status === 401) { | ||
| alert("잘못된 계정입니다, 다시 입력해주세요."); | ||
| } else { | ||
| alert("에러가 발생했습니다, 관리자에게 문의해주세요."); | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and add loading state
The sign-in handler could benefit from several improvements:
- Consistent async/await usage
- Proper error typing
- Loading state to prevent multiple submissions
+ const [isLoading, setIsLoading] = useState(false);
+
+ const handleSignIn = async () => {
+ if (isLoading) return;
+
+ try {
+ setIsLoading(true);
+ const res = await signIn({ email, password });
+ localStorage.setItem("FLOWBIT_ACT", res.data.accessToken);
+ setLogin(true);
+ navigate("/community");
+ } catch (error) {
+ if (error?.status === 401) {
+ alert("잘못된 계정입니다, 다시 입력해주세요.");
+ } else {
+ alert("에러가 발생했습니다, 관리자에게 문의해주세요.");
+ }
+ } finally {
+ setIsLoading(false);
+ }
+ };Committable suggestion skipped: line range outside the PR's diff.
| <div | ||
| id="content" | ||
| contentEditable={false} | ||
| dangerouslySetInnerHTML={{ __html: content }} | ||
| css={css` | ||
| ${DESIGN_SYSTEM_TEXT.B1} | ||
| color: ${DESIGN_SYSTEM_COLOR.GRAY_700}; | ||
| `} | ||
| > | ||
| {content} | ||
| </span> | ||
| ></div> |
There was a problem hiding this comment.
Security Risk: Unsafe HTML rendering
Using dangerouslySetInnerHTML without sanitization exposes the application to XSS attacks.
Consider using a sanitization library:
+import DOMPurify from 'dompurify';
-dangerouslySetInnerHTML={{ __html: content }}
+dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(content) }}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 235-236: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
There was a problem hiding this comment.
오 sanitize 이런게 있었구나?!
There was a problem hiding this comment.
@klmhyeonwoo 네, DOMPurify의 sanitize 함수를 사용하여 dangerouslySetInnerHTML 사용 시 보안 위험을 최소화할 수 있습니다. 이는 XSS 공격으로부터 애플리케이션을 보호하는 데 도움이 됩니다. 다음과 같이 적용하실 수 있습니다:
import DOMPurify from 'dompurify';
// ...
<div
id="content"
contentEditable={false}
dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(content) }}
css={css`
${DESIGN_SYSTEM_TEXT.B1}
color: ${DESIGN_SYSTEM_COLOR.GRAY_700};
`}
/>이를 통해 외부로부터 입력된 HTML 콘텐츠를 안전하게 렌더링할 수 있습니다.
목적
필수 리뷰어
작업 내용
profile이 존재하지 않았을 때에 대한 오류 처리src/components/layout/header.tsx
src/components/layout/index.tsx
src/api/index.ts
Axios Interceptor에서 글로벌 네비게이션을 사용할 수 있도록 수정401일 경우, 로그인 페이지로 이동할 수 있도록 이동테스트
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Style
Chores