feat: Better Grades Page #656
Conversation
Added date and grade score in search algorithm. Used some() instead of filter(). Added null check for grades. Imported GradeScore. Cleaned code for improved readability.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…hacal/Papillon into feat/better-grades-page
WalkthroughAjout de gardes et d'accès sécurisés pour gérer null/undefined dans la vue des notes, export public étendu avec Changes
Sequence Diagram(s)(aucun diagramme généré — changements principalement de robustesse et de typage, pas de nouveau flux multi-composants) Estimated code review effort🎯 3 (Modéré) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/(tabs)/grades/index.tsx (3)
186-193: Mutation potentielle des objets originaux.
[...subjects]crée une copie superficielle du tableau, mais les objetssubjectà l'intérieur restent des références aux originaux. Le tri desubject.gradesmute donc les données originales desubjects, ce qui peut causer des comportements inattendus.♻️ Proposition de correction
const sortedSubjects = useMemo(() => { - const subjectsCopy = [...subjects]; - - subjectsCopy.forEach((subject) => { - if (subject.grades) { - subject.grades.sort((a, b) => (b.givenAt?.getTime() ?? 0) - (a.givenAt?.getTime() ?? 0)); - } - }); + const subjectsCopy = subjects.map((subject) => ({ + ...subject, + grades: subject.grades + ? [...subject.grades].sort((a, b) => (b.givenAt?.getTime() ?? 0) - (a.givenAt?.getTime() ?? 0)) + : subject.grades, + }));
229-233: Le chaînage optionnel est redondant après le filtre.Après le filtrage à la ligne 230 qui garantit que
g !== undefined && g.givenAt !== undefined, le chaînage optionnelb?.givenAt?.getTime()à la ligne 231 est superflu. Ce n'est pas un bug, mais une simplification est possible.♻️ Simplification optionnelle
const sortedGrades = useMemo(() => { const gradesCopy = [...grades].filter(g => g !== undefined && g.givenAt !== undefined); - gradesCopy.sort((a, b) => (b?.givenAt?.getTime() ?? 0) - (a?.givenAt?.getTime() ?? 0)); + gradesCopy.sort((a, b) => b.givenAt!.getTime() - a.givenAt!.getTime()); return gradesCopy; }, [grades]);
392-393: Vérifications ternaires redondantes.Puisque la ligne 382 retourne déjà si
!grade, ces vérifications sont superflues à ce point du code.♻️ Simplification
-avgInfluence: grade ? getAvgInfluence(grade) : 0, -avgClass: grade ? getAvgClassInfluence(grade) : 0, +avgInfluence: getAvgInfluence(grade), +avgClass: getAvgClassInfluence(grade),
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/(tabs)/grades/index.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/(tabs)/grades/index.tsx (4)
utils/subjects/name.ts (1)
getSubjectName(6-23)ui/components/CompactGrade.tsx (1)
CompactGrade(29-166)utils/subjects/emoji.ts (1)
getSubjectEmoji(5-22)utils/subjects/colors.ts (1)
getSubjectColor(5-23)
🔇 Additional comments (5)
app/(tabs)/grades/index.tsx (5)
14-14: LGTM !L'import de
GradeScoreest correctement ajouté pour typerserviceRankà la ligne 134.
140-145: LGTM !Bonne programmation défensive pour gérer les réponses nulles ou incomplètes de l'API. Le retour anticipé évite les erreurs lors du traitement des données.
213-222: LGTM !Bonne gestion des cas limites avec le chaînage optionnel et les vérifications de nullité. La logique de tri par date de la dernière note est correcte.
238-268: LGTM ! Amélioration notable de la recherche.L'utilisation de
some()au lieu defilter()est plus performante. La normalisation et la recherche étendue (description, date, score) améliorent l'expérience utilisateur. Les vérifications défensives avec?? falsegarantissent un typage booléen correct.
303-305: LGTM !Le filtre avec garde de type
NonNullableassure que le composantAveragesreçoit un tableau propre sans valeursundefined.
TODO: **ADD TRANSLATION (Grade_Date_Error)**
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/(tabs)/grades/index.tsx (3)
136-169: Refresh bloqué possible:isRefreshingn’est pas remis àfalsesur l’early-return.Quand
!grades || !grades.subjects, vous faitesreturnaprèssetGradesLoading(false)mais sanssetIsRefreshing(false)— le pull-to-refresh peut rester “en cours”.Proposition de diff
if (!grades || !grades.subjects) { setGradesLoading(false); + setIsRefreshing(false); return; }
186-227: Tri: évitez de mutersubject.grades(mutation de state via copie superficielle).
subjectsCopy = [...subjects]ne clone pas les objets ni leurs tableaux;subject.grades.sort(...)trie “in-place” et peut muter l’état React.Proposition de diff
const sortedSubjects = useMemo(() => { - const subjectsCopy = [...subjects]; - - subjectsCopy.forEach((subject) => { - if (subject.grades) { - subject.grades.sort((a, b) => (b.givenAt?.getTime() ?? 0) - (a.givenAt?.getTime() ?? 0)); - } - }); + const subjectsCopy = subjects.map((subject) => ({ + ...subject, + grades: subject.grades + ? [...subject.grades].sort( + (a, b) => (b.givenAt?.getTime() ?? 0) - (a.givenAt?.getTime() ?? 0) + ) + : subject.grades, + }));
359-395: Risque d’effets de bord: éviter d’appelergetSubject*avec""(pollution du store).Avec
grade?.subjectId ?? ''puis...?.name || "", vous pouvez appelergetSubjectEmoji/getSubjectName/getSubjectColoravec une chaîne vide; d’aprèsutils/subjects/emoji.ts/colors.ts/name.ts, ça peut écrire dans le store (clé “vide”) et générer une couleur aléatoire inutile.Proposition de diff (calculer `subjectName` une seule fois + fallback sans side-effects)
- renderItem={({ item: grade }) => - <CompactGrade - emoji={getSubjectEmoji(getSubjectById(grade?.subjectId ?? '')?.name || "")} - title={getSubjectName(getSubjectById(grade?.subjectId ?? '')?.name || "")} + renderItem={({ item: grade }) => { + const subject = getSubjectById(grade?.subjectId ?? ''); + const subjectName = subject?.name; + const safeEmoji = subjectName ? getSubjectEmoji(subjectName) : '🤓'; + const safeTitle = subjectName ? getSubjectName(subjectName) : ''; + const safeColor = subjectName ? getSubjectColor(subjectName) : '#888888'; + + return ( + <CompactGrade + emoji={safeEmoji} + title={safeTitle} description={grade?.description || ""} score={grade?.studentScore?.value || 0} outOf={grade?.outOf?.value || 20} disabled={grade?.studentScore?.disabled} status={grade?.studentScore?.status} - color={getSubjectColor(getSubjectById(grade?.subjectId ?? '')?.name || "")} + color={safeColor} date={grade?.givenAt} hasMaxScore={grade?.studentScore?.value === grade?.maxScore?.value && !grade?.studentScore?.disabled} onPress={() => { if (!grade) return; // @ts-expect-error navigation types navigation.navigate('(modals)/grade', { grade: grade, subjectInfo: { - name: getSubjectName(getSubjectById(grade.subjectId)?.name || ""), - color: getSubjectColor(getSubjectById(grade.subjectId)?.name || ""), - emoji: getSubjectEmoji(getSubjectById(grade.subjectId)?.name || ""), - originalName: getSubjectById(grade.subjectId)?.name || "" + name: subjectName ? getSubjectName(subjectName) : '', + color: subjectName ? getSubjectColor(subjectName) : '#888888', + emoji: subjectName ? getSubjectEmoji(subjectName) : '🤓', + originalName: subjectName || '' }, avgInfluence: getAvgInfluence(grade), avgClass: getAvgClassInfluence(grade), }) }} /> - } + ); + }} />
🧹 Nitpick comments (1)
ui/components/CompactGrade.tsx (1)
13-27: Typage dedate: préférer une prop optionnelle plutôt queDate | undefined.
date: Date | undefinedoblige souvent les appelants à passer explicitementundefined. Undate?: Dateest généralement plus ergonomique, tout en conservant le même comportement runtime.Proposition de diff
interface CompactGradeProps { emoji: string; title: string; description: string; score: number; outOf: number; - date: Date | undefined; + date?: Date; disabled?: boolean; status?: string; onPress?: () => void, hasMaxScore?: boolean, color?: string; variant?: "normal" | "home"; skeleton?: boolean; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/(tabs)/grades/index.tsxui/components/CompactGrade.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
ui/components/CompactGrade.tsx (1)
utils/adjustColor.ts (1)
adjust(3-43)
app/(tabs)/grades/index.tsx (4)
utils/subjects/name.ts (1)
getSubjectName(6-23)ui/components/CompactGrade.tsx (1)
CompactGrade(29-174)utils/subjects/emoji.ts (1)
getSubjectEmoji(5-22)utils/subjects/colors.ts (1)
getSubjectColor(5-23)
🔇 Additional comments (4)
app/(tabs)/grades/index.tsx (4)
13-15: ImportGradeScore: OK si le type public a bien été étendu côté service.
229-233:sortedGrades: OK, mais vous excluez toutes les notes sansgivenAt.
301-307: Filtrage avantAverages: bon durcissement (évite lesundefined).
367-367:keyExtractorrobuste + gardeonPress: good.Also applies to: 381-382
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ui/components/CompactGrade.tsx (1)
116-123: Envisager une validation supplémentaire pour les objets Date invalides.Le garde conditionnel
date &&protège correctement contrenulletundefined. Cependant, si un objetDateinvalide est fourni (par exemple,new Date('invalid')), il passerait le test de vérité maistoLocaleDateString()pourrait produire des résultats inattendus.♻️ Proposition de validation défensive
{date && <Typography variant="body1" color={variant === "home" ? "secondary" : adjust(color, theme.dark ? 0.2 : -0.4)} nowrap skeleton={skeleton}> - {date.toLocaleDateString(i18n.language, { + {!isNaN(date.getTime()) ? date.toLocaleDateString(i18n.language, { day: "2-digit", month: "short", - })} + }) : t('Grade_Date_Error')} </Typography> }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/components/CompactGrade.tsx
🔇 Additional comments (1)
ui/components/CompactGrade.tsx (1)
19-19: Excellente amélioration défensive.Rendre la prop
dateoptionnelle est une bonne pratique qui permet de gérer les cas où la date n'est pas disponible, tout en évitant les erreurs d'exécution.
Added const variables for the subject `color`, `name`, `emoji`. One request not multiples.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/(tabs)/grades/index.tsx (2)
231-232: Filtrage robuste des notes avant tri.Le filtrage des notes
undefinedet de celles sansgivenAtavant le tri est une bonne pratique qui garantit la cohérence des données.♻️ Légère redondance (optionnel)
Étant donné que le filtre à la ligne 231 garantit que
givenAtexiste, l'optional chaining à la ligne 232 (?? 0) devient redondant. Cependant, cette défense en profondeur reste acceptable pour la robustesse.const gradesCopy = [...grades].filter(g => g !== undefined && g.givenAt !== undefined); -gradesCopy.sort((a, b) => (b?.givenAt?.getTime() ?? 0) - (a?.givenAt?.getTime() ?? 0)); +gradesCopy.sort((a, b) => b.givenAt.getTime() - a.givenAt.getTime());
238-269: Améliorations pertinentes de la recherche.L'ajout de la recherche dans les descriptions, dates et scores des notes (comme mentionné dans les objectifs du PR) est bien implémenté. L'utilisation de
some()au lieu defilter()est plus efficace, et l'optional chaining prévient les erreurs.⚡ Suggestion d'optimisation performance (optionnel)
Le formatage de la date à la ligne 260 à l'intérieur de la boucle
some()pourrait impacter les performances avec un grand nombre de notes. Envisagez de pré-formater les dates si nécessaire.return sortedSubjects.filter((subject) => { const subjectName = getSubjectName(subject.name).toLowerCase(); if (subjectName.includes(normalizedSearch)) { return true; } const hasMatchingGrade = subject.grades?.some((grade) => { const descriptionMatch = grade.description?.toLowerCase().includes(normalizedSearch) || false; - const dateMatch = grade.givenAt?.toLocaleDateString(i18n.language).includes(normalizedSearch) || false; + const dateMatch = grade.givenAt ? grade.givenAt.toLocaleDateString(i18n.language).includes(normalizedSearch) : false; const scoreMatch = grade.studentScore?.value.toString().includes(normalizedSearch) || false; return descriptionMatch || dateMatch || scoreMatch; }) ?? false; return hasMatchingGrade; });Note : Cette modification est mineure et la version actuelle reste acceptable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/(tabs)/grades/index.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/(tabs)/grades/index.tsx (4)
utils/subjects/name.ts (1)
getSubjectName(6-23)utils/subjects/emoji.ts (1)
getSubjectEmoji(5-22)utils/subjects/colors.ts (1)
getSubjectColor(5-23)ui/components/CompactGrade.tsx (1)
CompactGrade(29-166)
🔇 Additional comments (4)
app/(tabs)/grades/index.tsx (4)
14-14: LGTM !L'ajout de
GradeScoreaux imports est nécessaire pour le typage deserviceRank(ligne 134) et cohérent avec les modifications du fichier.
187-194: Bonne gestion défensive des valeurs null/undefined.Le tri des notes avec optional chaining (
grades?.[0],givenAt?.getTime() ?? 0) et la préservation de la référence originale quandgradesest undefined sont des améliorations robustes qui préviennent les erreurs d'exécution.
215-223: Tri des notes récentes correctement sécurisé.La logique gère proprement tous les cas limites (grades absents, dates undefined) et utilise l'optional chaining de manière cohérente.
305-305: Type guard correctement implémenté.Le filtrage avec prédicat de type
(g): g is NonNullable<typeof g>garantit que le composantAveragesreçoit uniquement des notes définies, renforçant la sécurité de type.
1 participant
Résumé des changements
Ajout de la
date et du scoredes notes dans l'algorithme de recherche des sujets. Utilisation desome()à la place defilter(). Ajout denull checkun peu partout dans le code pour les notes (il y avait plein d'erreurs). Ajout de l'import deGradeScore. Nettoyage du code pour une meilleure lisibilité.Summary by CodeRabbit
Notes de publication
Améliorations
Corrections de bogues
✏️ Tip: You can customize this high-level summary in your review settings.