히스토리 페이지 개발#68
Conversation
Summary of ChangesHello @HWAN0218, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 사용자가 완료한 작업을 월별 및 카테고리별로 쉽게 확인하고 관리할 수 있는 히스토리 페이지를 도입합니다. 사용자는 자신의 팀과 관련된 완료된 작업을 조회하고, 특정 작업의 상세 내용을 확인하며, 댓글을 작성하거나 작업을 수정/삭제할 수 있습니다. 이 페이지는 다양한 화면 크기에 맞춰 최적의 사용자 경험을 제공하도록 반응형으로 설계되었습니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new history page, complete with a responsive layout for desktop and mobile, including a sidebar and mobile navigation. It integrates new API hooks for fetching user data, group details, and done tasks, as well as mutations for task comments, completion, and deletion. The page displays completed tasks grouped by date, allows filtering by task list categories, and provides a detailed view for each task. Review comments highlight concerns regarding the extensive use of !important in the new CSS module, suggesting refactoring for better maintainability and specificity. Additionally, several comments point out potential type safety issues with as number assertions and string-based checks in the JavaScript logic, recommending more robust type handling and element identification methods.
| /* ✅ list 페이지와 동일: aside는 visible, 내용 영역만 hidden */ | ||
| :global([class*='Sidebar-module__'][class*='sidebar']) { | ||
| overflow: visible !important; | ||
| } | ||
| :global([class*='Sidebar-module__'][class*='content']) { | ||
| overflow: hidden !important; | ||
| } |
There was a problem hiding this comment.
Using !important extensively can lead to specificity issues and make CSS harder to maintain and debug. Consider refactoring these styles to avoid !important by adjusting the component's structure or using more specific selectors.
| /* ✅ 텍스트 줄바꿈/넘침 방지 (list 동일) */ | ||
| :global([class*='Sidebar-module__'] button), | ||
| :global([class*='Sidebar-module__'] a), | ||
| :global([class*='Sidebar-module__'] span), | ||
| :global([class*='Sidebar-module__'] p) { | ||
| max-width: 100% !important; | ||
| overflow: hidden !important; | ||
| text-overflow: ellipsis !important; | ||
| white-space: nowrap !important; | ||
| } |
There was a problem hiding this comment.
Similar to the previous comment, the use of !important here for text overflow properties can create maintenance challenges. It's generally better to control these styles through proper component hierarchy and CSS specificity.
| @media (min-width: 1024px) and (max-width: 1199px) { | ||
| .desktopSidebar { | ||
| display: flex !important; | ||
| } | ||
| .mobileGnb { | ||
| display: none !important; | ||
| } | ||
| :global([class*='Sidebar-module__'][class*='sidebar']) { | ||
| display: flex !important; | ||
| } |
There was a problem hiding this comment.
The use of !important in media queries can be particularly problematic. If these styles are intended to override existing ones, try to achieve the desired effect with more specific selectors or by ensuring the order of CSS rules is correct.
| :global(.TeamHeader-module__H3kcRq__container) { | ||
| width: 100%; | ||
| max-width: 1200px; | ||
| margin-left: auto !important; | ||
| margin-right: auto !important; | ||
| } |
There was a problem hiding this comment.
Using !important for margin-left and margin-right can make it difficult to override these properties later. Consider if there's a way to achieve the centering without !important, perhaps by using display: flex on the parent and justify-content: center.
| :global(.Chip-module__j4Ti-q__chip.Chip-module__j4Ti-q__small) { | ||
| border: 1px solid var(--color-background-tertiary, #e2e8f0) !important; | ||
| } |
There was a problem hiding this comment.
The !important declaration here for border on the Chip component might be unnecessary if the base styles are managed correctly. Review if this override is strictly required or if it can be achieved through better CSS organization.
| const iso = t.doneAt ?? t.date ?? ''; | ||
| const dayKey = iso ? iso.slice(0, 10) : '1970-01-01'; |
There was a problem hiding this comment.
The default dayKey of '1970-01-01' for tasks without doneAt or date might lead to unexpected grouping or display if such tasks exist. It's good to have a fallback, but consider if these tasks should be explicitly excluded or if a more descriptive dayKey (e.g., 'Undated Tasks') would be better for debugging/display.
| if (filteredTasks.length === 0) return 0; | ||
| if (filteredTasks.some((t) => t.id === selectedTaskId)) return selectedTaskId; | ||
| return filteredTasks[0].id; | ||
| }, [filteredTasks, selectedTaskId]); |
There was a problem hiding this comment.
The logic for effectiveSelectedTaskId ensures a task is always selected if filteredTasks is not empty. This is a good fallback, but if filteredTasks becomes empty while selectedTaskId still holds a value, effectiveSelectedTaskId will become 0. Ensure that 0 is correctly interpreted as 'no task selected' or handle this case explicitly where effectiveSelectedTaskId is used.
| taskId: c.taskId ?? effectiveSelectedTaskId, | ||
| userId: c.userId ?? c.user?.id ?? 0, |
There was a problem hiding this comment.
The taskId and userId properties are asserted with ?? effectiveSelectedTaskId and ?? c.user?.id ?? 0. While effectiveSelectedTaskId is a number, c.user?.id could be undefined. The ?? 0 fallback is good, but ensure that 0 is a valid default for userId in all contexts where UiComment is used.
| window.setTimeout(() => setDetailMounted(false), 260); | ||
| } |
There was a problem hiding this comment.
The window.setTimeout with a hardcoded delay of 260ms is tied to the CSS transition duration. If the CSS transition duration changes, this JavaScript delay will need to be updated manually. Consider using onTransitionEnd event if precise synchronization is critical, or define the duration as a constant in a shared place.
| <div style={{ position: 'relative' }}> | ||
| <TaskListItem | ||
| title={task.name} | ||
| date={formatKoreanDateFromIso(`${group.dayKey}T00:00:00.000Z`)} | ||
| checked={!!task.doneAt} |
There was a problem hiding this comment.
The date prop for TaskDetailCard is cast as string after a series of nullish coalescing operators. While new Date().toISOString() will always return a string, if selectedTask.doneAt or selectedTask.date were somehow not strings, this cast would hide a potential type mismatch. Ensure that the upstream data types are consistently strings or handle potential non-string values more explicitly.
2 participants
Summary
Issue
Scope
포함
특이사항