Skip to content

chore: fixed goal percentage#639

Merged
LucasCharrier merged 1 commit intomasterfrom
task/fix-goals-percentage
Nov 26, 2025
Merged

chore: fixed goal percentage#639
LucasCharrier merged 1 commit intomasterfrom
task/fix-goals-percentage

Conversation

@LucasCharrier
Copy link
Copy Markdown
Contributor

No description provided.

@revu-bot revu-bot bot requested a review from revu-bot November 26, 2025 10:11
@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Collaborator

@revu-bot revu-bot left a comment

Choose a reason for hiding this comment

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

Summary

This PR swaps the array indices used to display percentages for "Oui" (Yes) and "Non" (No) labels in the goals pie chart. The change suggests the original implementation had the indices reversed.

Key Observations

  • Index swap: sections[1] and sections[5] are swapped between the two labels
  • Potential bug fix: This appears to correct a mismatch where "Oui" was showing the percentage for value 1 (which represents "Non" based on the parialsColors mapping)
  • Data mapping context: According to the code, records are mapped with value: record.value === true ? 5 : 1, meaning:
    • true → value 5 → should display as "Oui"
    • false → value 1 → should display as "Non"

Issues Identified

Severity Issue Impact
IMPORTANT Fragile array indexing pattern High maintenance risk and potential for similar bugs
IMPORTANT Missing validation for sections object Runtime errors if data structure changes

The core fix appears correct, but the implementation pattern is brittle and error-prone.

Comment on lines 159 to 170
@@ -166,7 +166,7 @@ const GoalPie = ({ title, records }) => {
</View>

<Text className={mergeClassNames(typography.textSmSemibold, "text-cnam-primary-800")}>
{Math.round(sections?.[5]?.percentage || 0)}%
{Math.round(sections?.[1]?.percentage || 0)}%
</Text>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[IMPORTANT] Fragile magic number indexing creates maintenance risk

Why this matters:

  • Direct array indexing with magic numbers (sections?.[5]) is error-prone, as evidenced by this bug fix
  • The relationship between the value (5) and its meaning ("Oui"/true) is implicit and scattered across the codebase
  • Future developers may not understand why 5 means "yes" and 1 means "no"
  • This pattern already caused one bug and will likely cause more

Recommended approach:

Define semantic constants at the top of the file:

const RECORD_VALUES = {
  YES: 5,
  NO: 1,
  EMPTY: 0,
} as const;

Then update the mapping logic:

const _records = records
  .filter((record) => chartDates.includes(record.date))
  .map((record) => ({
    ...record,
    value: record.value === true ? RECORD_VALUES.YES : RECORD_VALUES.NO,
  }));

And use semantic access in the display:

{Math.round(sections?.[RECORD_VALUES.YES]?.percentage || 0)}%

This makes the code self-documenting and prevents future index confusion.

Suggested change
<Text className={mergeClassNames(typography.textSmSemibold, "text-cnam-primary-800")}>
{Math.round(sections?.[5]?.percentage || 0)}% {/* YES value */}
</Text>
</View>
<View className="flex flex-row mt-2 items-center">
<View style={{ backgroundColor: parialsColors[1].color }} className={`flex justify-center items-center h-10 w-10 mr-1 rounded-full`}>
<Text className="text-cnam-primary-800 text-sm">Non</Text>
</View>
<Text className={mergeClassNames(typography.textSmSemibold, "text-cnam-primary-800")}>
{Math.round(sections?.[1]?.percentage || 0)}% {/* NO value */}
</Text>


<Text className={mergeClassNames(typography.textSmSemibold, "text-cnam-primary-800")}>
{Math.round(sections?.[5]?.percentage || 0)}%
{Math.round(sections?.[1]?.percentage || 0)}%
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[IMPORTANT] Missing type safety for sections object structure

Why this matters:

  • The sections state is typed as {} (empty object), providing no type safety
  • Optional chaining (?.) masks potential runtime issues if the data structure is incorrect
  • No validation ensures that sections[1] and sections[5] actually exist before rendering

Recommended solution:

Define a proper TypeScript interface:

interface SectionData {
  score: number;
  total: number;
  count: number;
  percentage: number;
  color: { color: string; symbol?: string };
}

type Sections = Record<number, SectionData>;

Then update the state declaration:

const [sections, setSections] = useState<Sections>({});

This provides compile-time safety and better IDE support, making it harder to introduce bugs like the one this PR fixes.

@LucasCharrier LucasCharrier merged commit 38cd6de into master Nov 26, 2025
7 of 10 checks passed
@LucasCharrier LucasCharrier deleted the task/fix-goals-percentage branch November 26, 2025 10:15
@tokenbureau
Copy link
Copy Markdown

tokenbureau bot commented Feb 9, 2026

🎉 This PR is included in version 1.129.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@tokenbureau tokenbureau bot added the released label Feb 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants