Website: Dedicated page for all survey answers #928
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Direct link to preview of page: https://public-git-gavrill-raw-survey-aggregation-social-income.vercel.app/en/ch/survey/responses |
|
Visit the preview URL for this PR (updated for commit d94d428): https://si-admin-staging--pr928-gavrill-raw-survey-a-ryohjgr2.web.app (expires Sat, 04 Jan 2025 14:52:40 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b7b0969384059dce6ea8fad1ee1d1737e54e6676 |
Gavriil-Tzortzakis
force-pushed
the
gavrill/raw-survey-aggregation
branch
from
1751b84 to
7fa6cae
Compare
Gavriil-Tzortzakis
force-pushed
the
gavrill/raw-survey-aggregation
branch
from
8b15f4f to
4ba3261
Compare
Gavriil-Tzortzakis
force-pushed
the
gavrill/raw-survey-aggregation
branch
from
2774d36 to
c6ea5d5
Compare
Gavriil-Tzortzakis
force-pushed
the
gavrill/raw-survey-aggregation
branch
from
b67560b to
7e5dbb5
Compare
Gavriil-Tzortzakis
force-pushed
the
gavrill/raw-survey-aggregation
branch
from
7e5dbb5 to
5adc6ce
Compare
Gavriil-Tzortzakis
force-pushed
the
gavrill/raw-survey-aggregation
branch
from
5adc6ce to
c946bcd
Compare
ssandino
changed the title
📝 WalkthroughWalkthroughThis pull request introduces significant enhancements to survey-related functionality across various files. It includes a new localization JSON for survey responses, a Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 review |
✅ Actions performedReview triggered.
|
![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 (6)
website/src/app/[lang]/[region]/(website)/survey/responses/page.tsx (2)
20-28: Improve variable naming and data transformation clarity.The variable name 'temp' is not descriptive of its purpose. Consider renaming it to something more meaningful like 'surveyStats' or 'rawSurveyData'.
-const temp = surveyStatsCalculator.data; -const allSurveyData = Object.values(SurveyQuestionnaire).map((it) => temp.find((survey) => survey.type == it)); +const rawSurveyData = surveyStatsCalculator.data; +const allSurveyData = Object.values(SurveyQuestionnaire).map((surveyType) => + rawSurveyData.find((survey) => survey.type === surveyType) +);
59-60: Simplify Fragment key generation.The key concatenation can be simplified using template literals.
-<Fragment key={selectedSurvey + key + 'statistics'}> +<Fragment key={`${selectedSurvey}-${key}-statistics`}>shared/src/utils/stats/SurveyStatsCalculator.ts (4)
37-40: Simplify 'return await' to 'return' in async functionsThe
return awaitinside an async function is redundant. Simplify it toreturnto improve code clarity.Apply this diff:
-return await firestoreAdmin +return firestoreAdmin
42-42: Use 'const' instead of 'let' for variables that are not reassignedThe variable
ignoredis not reassigned after its initial assignment. Replaceletwithconstto indicate that the value doesn't change.Apply this diff:
-let ignored = ['pageNo']; +const ignored = ['pageNo'];
43-43: Use 'const' instead of 'let' for 'surveysData'Since
surveysDatais not reassigned, useconstto enhance code readability and maintainability.Apply this diff:
-let surveysData = documents.flatMap((snapshot) => snapshot.docs).map((survey) => survey.data()); +const surveysData = documents.flatMap((snapshot) => snapshot.docs).map((survey) => survey.data());
28-30: Complete the JSDoc comment for the 'build' methodThe JSDoc comment for the
buildmethod is incomplete. Provide a description of the method and its parameters to improve code documentation.Apply this diff:
/** + * Builds a SurveyStatsCalculator instance by aggregating survey statistics from Firestore. * @param firestoreAdmin FirestoreAdmin instance for database operations. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
shared/locales/en/website-responses.json(1 hunks)shared/src/utils/stats/SurveyStatsCalculator.ts(1 hunks)ui/src/components/tabs.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/survey/responses/page.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- shared/locales/en/website-responses.json
🔇 Additional comments (3)
ui/src/components/tabs.tsx (2)
11-11: Implementation is consistent and clean
The simplified className handling is consistently applied across all tab components while maintaining proper type safety and ref forwarding.
Also applies to: 17-17, 23-23
11-11: Verify className handling in consuming components
The removal of the cn utility might affect components that rely on className merging functionality.
Also applies to: 17-17, 23-23
✅ Verification successful
No issues with className handling in consuming components
The search results show that the consuming components in website/src/app/[lang]/[region]/(website)/survey/responses/page.tsx are using single className values for TabsList, TabsTrigger, and TabsContent components. The className values are being passed directly without any merging functionality, so the removal of the cn utility does not affect these implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using these tab components with multiple className values
rg -l "TabsList|TabsTrigger|TabsContent" | xargs rg "className=.*\s+.*"
Length of output: 2885
website/src/app/[lang]/[region]/(website)/survey/responses/page.tsx (1)
1-19: LGTM! Imports and configuration are well-organized.
The imports are logically grouped and the revalidation time of 1 hour is appropriate for survey data that doesn't change frequently.
website/src/app/[lang]/[region]/(website)/survey/responses/page.tsx
Outdated
Show resolved
Hide resolved
website/src/app/[lang]/[region]/(website)/survey/responses/page.tsx
Outdated
Show resolved
Hide resolved
Gavriil-Tzortzakis
force-pushed
the
gavrill/raw-survey-aggregation
branch
2 times, most recently
from
5c97059 to
7c7d6ee
Compare
ssandino
changed the title
Gavriil-Tzortzakis
force-pushed
the
gavrill/raw-survey-aggregation
branch
from
7c7d6ee to
fcb398c
Compare
Gavriil-Tzortzakis
force-pushed
the
gavrill/raw-survey-aggregation
branch
from
1fd95c5 to
46fc290
Compare
Gavriil-Tzortzakis
force-pushed
the
gavrill/raw-survey-aggregation
branch
from
46fc290 to
7608cd5
Compare
Gavriil-Tzortzakis
requested review from
andrashee,
mkue and
socialincome-dev
as code owners
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
shared/locales/en/website-responses.json(1 hunks)shared/locales/en/website-survey.json(1 hunks)shared/locales/it/website-survey.json(1 hunks)shared/locales/kri/website-survey.json(1 hunks)shared/src/types/question.ts(1 hunks)shared/src/utils/stats/SurveyStatsCalculator.test.ts(1 hunks)shared/src/utils/stats/SurveyStatsCalculator.ts(1 hunks)ui/src/components/tabs.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/survey/responses/barchart-survey-response-component.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/survey/responses/page.tsx(1 hunks)website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questions.ts(21 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- shared/locales/en/website-responses.json
- shared/locales/en/website-survey.json
- ui/src/components/tabs.tsx
- website/src/app/[lang]/[region]/(website)/survey/responses/page.tsx
- shared/locales/it/website-survey.json
- shared/locales/kri/website-survey.json
- shared/src/utils/stats/SurveyStatsCalculator.test.ts
- website/src/app/[lang]/[region]/(website)/survey/responses/barchart-survey-response-component.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
shared/src/utils/stats/SurveyStatsCalculator.ts
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 72-72: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 80-80: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questions.ts
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 60-60: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
[error] 350-350: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
🪛 Gitleaks (8.21.2)
shared/src/types/question.ts
47-47: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
235-235: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (5)
shared/src/utils/stats/SurveyStatsCalculator.ts (1)
40-41: 🛠️ Refactor suggestion
Replace 'this' with class name in static methods
Using this in static methods can be confusing. Use the class name SurveyStatsCalculator instead.
-const surveysData = await this.fetchAndProcessSurveys(firestoreAdmin, recipients);
-const { typeCounts, aggregatedData, oldestDate } = this.aggregateSurveyData(surveysData);
+const surveysData = await SurveyStatsCalculator.fetchAndProcessSurveys(firestoreAdmin, recipients);
+const { typeCounts, aggregatedData, oldestDate } = SurveyStatsCalculator.aggregateSurveyData(surveysData);Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questions.ts (2)
350-354: 🛠️ Refactor suggestion
Improve type safety in translateChoices function
Replace any[] and String with more specific types.
-export const translateChoices = (t: TranslateFunction, choices: any[], choicesTranslationKey: String) =>
+export const translateChoices = (t: TranslateFunction, choices: string[], choicesTranslationKey: string): Array<{ value: string; text: string }> =>
choices.map((key) => ({
value: key,
text: t(choicesTranslationKey + '.' + key),
}));Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 350-350: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
60-71: 🛠️ Refactor suggestion
Improve type safety and optional chaining
Two improvements needed:
- Replace generic
Objectreturn type with a specific interface - Use consistent optional chaining
-function getSimpleMapping(question: Question, t: TranslateFunction): Object {
+interface QuestionMapping {
+ type: string;
+ name: string;
+ title: string;
+ description?: string;
+ choices?: Array<{ value: string; text: string }>;
+}
+
+function getSimpleMapping(question: Question, t: TranslateFunction): QuestionMapping {
return {
type: question.type,
name: question.name,
title: t(question.translationKey),
- description: question.descriptionTranslationKey && t(question.descriptionTranslationKey),
+ description: question.descriptionTranslationKey?.t(question.descriptionTranslationKey),
choices:
- question.choices && question.choices.length
+ question.choices?.length
? translateChoices(t, question.choices, question.choicesTranslationKey!)
: undefined,
};
}Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 60-60: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
shared/src/types/question.ts (2)
271-271: LGTM!
The Map implementation with string keys is correct and type-safe.
1-8: 🛠️ Refactor suggestion
Improve type safety in Question interface
The choices property using any[] reduces type safety.
export interface Question {
type: QuestionInputType;
name: string;
- choices: any[];
+ choices: (string | boolean | number)[];
choicesTranslationKey?: string;
translationKey: string;
descriptionTranslationKey?: string;
}Likely invalid or redundant comment.
|
When I run your local branch and visit http://localhost:3001/en/ch/survey/responses, I get the following error: 
@Gavriil-Tzortzakis any hints, how to get it running locally? Or does it work on yours @mkue ? |
Gavriil-Tzortzakis
force-pushed
the
gavrill/raw-survey-aggregation
branch
from
7608cd5 to
d9e7472
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
website/src/app/[lang]/[region]/(website)/survey/responses/barchart-survey-response-component.tsx (3)
10-10: Consider modernizing the component implementationConsider these improvements:
- Convert to a functional component using React hooks
- Add prop validation using PropTypes or TypeScript's required modifier
-export default class BarchartSurveyResponseComponent extends Component<{ data: ChartData[] }> { +interface BarchartSurveyResponseProps { + data: ChartData[]; +} + +export default function BarchartSurveyResponseComponent({ data }: BarchartSurveyResponseProps) {
16-16: Add fallback color for CSS variableProvide a fallback color in case the CSS variable is not defined.
-let fillColor = 'hsl(var(--foreground))'; +let fillColor = 'hsl(var(--foreground, 0 0% 0%))';
36-59: Consider dynamic margins for better responsivenessThe fixed right margin of 100px might not be optimal for all screen sizes. Consider making it responsive.
-margin={{ top: 20, right: 100, bottom: 20, left: 20 }} +margin={{ + top: 20, + right: Math.max(50, Math.min(window.innerWidth * 0.1, 100)), + bottom: 20, + left: 20 +}}shared/src/utils/stats/SurveyStatsCalculator.ts (2)
96-96: Improve type safety for response parameterThe
responseparameter is typed asany. Consider creating a union type for supported response types.-response: any, +response: string | string[],
50-59: Consider batch processing for large datasetsFetching all surveys at once could lead to performance issues with large datasets. Consider implementing pagination or batch processing.
website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questions.ts (1)
67-69: Use optional chaining for better null safetyThe condition can be simplified using optional chaining.
-question.choices && question.choices.length +question.choices?.length🧰 Tools
🪛 Biome (1.9.4)
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
shared/locales/en/website-responses.json(1 hunks)shared/locales/en/website-survey.json(1 hunks)shared/locales/it/website-survey.json(1 hunks)shared/locales/kri/website-survey.json(1 hunks)shared/src/types/question.ts(1 hunks)shared/src/utils/stats/SurveyStatsCalculator.test.ts(1 hunks)shared/src/utils/stats/SurveyStatsCalculator.ts(1 hunks)ui/src/components/badge.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/survey/responses/barchart-survey-response-component.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/survey/responses/opacity.css(1 hunks)website/src/app/[lang]/[region]/(website)/survey/responses/page.tsx(1 hunks)website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questionnaires.ts(4 hunks)website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questions.ts(21 hunks)
✅ Files skipped from review due to trivial changes (1)
- website/src/app/[lang]/[region]/(website)/survey/responses/opacity.css
🚧 Files skipped from review as they are similar to previous changes (6)
- shared/locales/en/website-survey.json
- shared/locales/en/website-responses.json
- shared/locales/it/website-survey.json
- shared/locales/kri/website-survey.json
- shared/src/utils/stats/SurveyStatsCalculator.test.ts
- website/src/app/[lang]/[region]/(website)/survey/responses/page.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
shared/src/utils/stats/SurveyStatsCalculator.ts
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 72-72: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 80-80: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questions.ts
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 60-60: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
[error] 350-350: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
🪛 Gitleaks (8.21.2)
shared/src/types/question.ts
47-47: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
235-235: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (8)
ui/src/components/badge.tsx (1)
15-15: LGTM! New accent variant follows component patterns.
The implementation is clean and consistent with existing variants. Consider verifying the visual appearance across different color modes (light/dark) to ensure proper contrast ratios.
✅ Verification successful
New accent variant is properly implemented and used across the codebase
The accent variant is well-integrated with:
- Proper CSS custom properties defined in
ui/src/globals.css - Consistent usage in multiple components including login and survey pages
- Correct theme configuration in
ui/tailwind.config.ts - Matching hover and focus states across components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of the new accent variant and related styling
# to ensure proper implementation across the codebase
# Check for usage of the accent variant
rg -l "variant=\"accent\"" --type tsx
# Check for accent color definitions in theme
rg "accent" --type css --type scss
Length of output: 129
Script:
#!/bin/bash
# Let's try without file type restrictions since the extensions might be different
# Check for usage of the accent variant
rg -l "variant=\"accent\""
# Check for accent-related styling in any file
rg "accent.*foreground|accent.*background|accent:.*bg-"
# Find all potential theme/style files
fd "theme|style|tailwind"
Length of output: 5323
website/src/app/[lang]/[region]/(website)/survey/responses/barchart-survey-response-component.tsx (1)
4-4: Verify the opacity.css dependency
The imported CSS file's contents should be reviewed to ensure it contains the necessary styles.
✅ Verification successful
CSS file contains valid gradient opacity styles for recharts
The opacity.css file exists and contains a valid CSS class that applies a gradient opacity effect to recharts components using Tailwind's @apply directive. The styles are properly scoped to .recharts-wrapper and implement a transparent-to-background gradient on the right side.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the opacity.css file exists and view its contents
cat website/src/app/[lang]/[region]/\(website\)/survey/responses/opacity.css
Length of output: 239
website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questionnaires.ts (1)
47-47: LGTM: Translation function properly passed to page components
The changes consistently pass the translation function to schoolAttendancePage and skippingMealsPage across all questionnaire types.
Also applies to: 50-50, 86-86, 89-89, 101-101, 104-104
website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questions.ts (2)
350-354: 🛠️ Refactor suggestion
Improve type safety in translateChoices
The function uses any[] and String types, which can be more specific.
-export const translateChoices = (t: TranslateFunction, choices: any[], choicesTranslationKey: String) =>
+export const translateChoices = (t: TranslateFunction, choices: string[], choicesTranslationKey: string): Array<{ value: string; text: string }> =>Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 350-350: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
60-71: 🛠️ Refactor suggestion
Improve type safety in getSimpleMapping
The function uses Object as return type, which is too broad.
-function getSimpleMapping(question: Question, t: TranslateFunction): Object {
+interface QuestionMapping {
+ type: string;
+ name: string;
+ title: string;
+ description?: string;
+ choices?: Array<{ value: string; text: string }>;
+}
+
+function getSimpleMapping(question: Question, t: TranslateFunction): QuestionMapping {Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 60-60: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
shared/src/types/question.ts (3)
1-8: Improve type safety for Question interface
The choices property using any[] reduces type safety.
export interface Question {
type: QuestionInputType;
name: string;
- choices: any[];
+ choices: (string | boolean | number)[];
choicesTranslationKey?: string;
translationKey: string;
descriptionTranslationKey?: string;
}10-15: LGTM!
The enum values are well-defined and consistent.
238-271: LGTM!
The questions array and dictionary implementation provides efficient question lookup by name.
...te/src/app/[lang]/[region]/(website)/survey/responses/barchart-survey-response-component.tsx
Outdated
Show resolved
Hide resolved
Gavriil-Tzortzakis
force-pushed
the
gavrill/raw-survey-aggregation
branch
from
d9e7472 to
1de0447
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
shared/locales/en/website-responses.json(1 hunks)shared/locales/en/website-survey.json(1 hunks)shared/locales/it/website-survey.json(1 hunks)shared/locales/kri/website-survey.json(1 hunks)shared/src/types/question.ts(1 hunks)shared/src/utils/stats/SurveyStatsCalculator.test.ts(1 hunks)shared/src/utils/stats/SurveyStatsCalculator.ts(1 hunks)ui/src/components/badge.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/survey/responses/barchart-survey-response-component.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/survey/responses/opacity.css(1 hunks)website/src/app/[lang]/[region]/(website)/survey/responses/page.tsx(1 hunks)website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questionnaires.ts(4 hunks)website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questions.ts(21 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- ui/src/components/badge.tsx
- website/src/app/[lang]/[region]/(website)/survey/responses/opacity.css
- shared/locales/en/website-responses.json
- shared/locales/en/website-survey.json
- shared/locales/it/website-survey.json
- shared/locales/kri/website-survey.json
- shared/src/utils/stats/SurveyStatsCalculator.test.ts
- website/src/app/[lang]/[region]/(website)/survey/responses/barchart-survey-response-component.tsx
- website/src/app/[lang]/[region]/(website)/survey/responses/page.tsx
- website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questionnaires.ts
🧰 Additional context used
🪛 Biome (1.9.4)
website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questions.ts
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 60-60: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
[error] 350-350: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
shared/src/utils/stats/SurveyStatsCalculator.ts
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 72-72: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 80-80: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🪛 Gitleaks (8.21.2)
shared/src/types/question.ts
47-47: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
235-235: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (6)
website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questions.ts (2)
350-354: Improve type safety in translateChoices
Previous review comment still applies. The function needs proper TypeScript types for parameters and return value.
🧰 Tools
🪛 Biome (1.9.4)
[error] 350-350: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
Line range hint 77-343: LGTM! Clean and consistent implementation
The page components consistently use getSimpleMapping, making the code more maintainable and reducing duplication.
shared/src/utils/stats/SurveyStatsCalculator.ts (2)
40-41: 🛠️ Refactor suggestion
Replace 'this' with class name in static methods
Using this in static methods can be confusing. Use the class name SurveyStatsCalculator instead.
-const surveysData = await this.fetchAndProcessSurveys(firestoreAdmin, recipients);
-const { typeCounts, aggregatedData, oldestDate } = this.aggregateSurveyData(surveysData);
+const surveysData = await SurveyStatsCalculator.fetchAndProcessSurveys(firestoreAdmin, recipients);
+const { typeCounts, aggregatedData, oldestDate } = SurveyStatsCalculator.aggregateSurveyData(surveysData);Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
96-97: 🛠️ Refactor suggestion
Improve type safety for response parameter
Using any reduces type safety. Consider using a union type based on the supported question types.
-response: any,
+response: string | string[],Likely invalid or redundant comment.
shared/src/types/question.ts (2)
1-8: 🛠️ Refactor suggestion
Improve type safety for Question interface
The choices property using any[] reduces type safety. Consider using a more specific type.
export interface Question {
type: QuestionInputType;
name: string;
- choices: any[];
+ choices: (string | boolean | number)[];
choicesTranslationKey?: string;
translationKey: string;
descriptionTranslationKey?: string;
}Likely invalid or redundant comment.
230-236:
Add choices for RANKING question type
The RANKING question type requires choices to be ranked, but the choices array is empty. Consider reusing the choices from the SPENDING question since they share the same context.
export const RANKING: Question = {
type: QuestionInputType.RANKING,
name: 'spendingRankedV1',
- choices: [],
+ choices: ['education', 'food', 'housing', 'healthCare', 'mobility', 'saving', 'investment'],
translationKey: 'survey.questions.spendingRankingTitleV1',
descriptionTranslationKey: 'survey.questions.spendingRankingDescV1',
};Likely invalid or redundant comment.
🧰 Tools
🪛 Gitleaks (8.21.2)
235-235: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| function getSimpleMapping(question: Question, t: TranslateFunction): Object { | ||
| return { | ||
| type: question.type, | ||
| name: question.name, | ||
| title: t(question.translationKey), | ||
| description: question.descriptionTranslationKey && t(question.descriptionTranslationKey), | ||
| choices: | ||
| question.choices && question.choices.length | ||
| ? translateChoices(t, question.choices, question.choicesTranslationKey!) | ||
| : undefined, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and null handling
Three improvements needed:
- Define specific return type
- Improve optional chaining
- Handle potential null choicesTranslationKey
-function getSimpleMapping(question: Question, t: TranslateFunction): Object {
+interface QuestionMapping {
+ type: string;
+ name: string;
+ title: string;
+ description?: string;
+ choices?: Array<{ value: string; text: string }>;
+}
+
+function getSimpleMapping(question: Question, t: TranslateFunction): QuestionMapping {
return {
type: question.type,
name: question.name,
title: t(question.translationKey),
- description: question.descriptionTranslationKey && t(question.descriptionTranslationKey),
+ description: question.descriptionTranslationKey ? t(question.descriptionTranslationKey) : undefined,
choices:
- question.choices && question.choices.length
+ question.choices?.length
? translateChoices(t, question.choices, question.choicesTranslationKey!)
: undefined,
};
}📝 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.
| function getSimpleMapping(question: Question, t: TranslateFunction): Object { | |
| return { | |
| type: question.type, | |
| name: question.name, | |
| title: t(question.translationKey), | |
| description: question.descriptionTranslationKey && t(question.descriptionTranslationKey), | |
| choices: | |
| question.choices && question.choices.length | |
| ? translateChoices(t, question.choices, question.choicesTranslationKey!) | |
| : undefined, | |
| }; | |
| } | |
| interface QuestionMapping { | |
| type: string; | |
| name: string; | |
| title: string; | |
| description?: string; | |
| choices?: Array<{ value: string; text: string }>; | |
| } | |
| function getSimpleMapping(question: Question, t: TranslateFunction): QuestionMapping { | |
| return { | |
| type: question.type, | |
| name: question.name, | |
| title: t(question.translationKey), | |
| description: question.descriptionTranslationKey ? t(question.descriptionTranslationKey) : undefined, | |
| choices: | |
| question.choices?.length | |
| ? translateChoices(t, question.choices, question.choicesTranslationKey!) | |
| : undefined, | |
| }; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 60-60: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
Gavriil-Tzortzakis
force-pushed
the
gavrill/raw-survey-aggregation
branch
from
1de0447 to
bf8d50c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
shared/src/types/question.ts (1)
1-271: Consider grouping related questions and documenting versioning strategy
- Related questions (e.g., HAPPIER, HAPPIER_COMMENT) could be grouped into objects or namespaces for better organization.
- The 'V1' suffix suggests versioning plans. Consider documenting the versioning strategy for future maintainers.
🧰 Tools
🪛 Gitleaks (8.21.2)
47-47: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
235-235: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
shared/src/utils/stats/SurveyStatsCalculator.ts (1)
73-76: Simplify date comparison logicThe current implementation can be simplified using optional chaining and a more concise comparison.
-oldestDate = - survey.completed_at?.toDate() && survey.completed_at?.toDate() < oldestDate - ? survey.completed_at.toDate() - : oldestDate; +const completedDate = survey.completed_at?.toDate(); +if (completedDate && completedDate < oldestDate) { + oldestDate = completedDate; +}website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questions.ts (1)
354-354: Use template literal for translation keyReplace string concatenation with template literal for better readability.
-text: t(choicesTranslationKey + '.' + key), +text: t(`${choicesTranslationKey}.${key}`),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
shared/locales/en/website-responses.json(1 hunks)shared/locales/en/website-survey.json(1 hunks)shared/locales/it/website-survey.json(1 hunks)shared/locales/kri/website-survey.json(1 hunks)shared/src/types/question.ts(1 hunks)shared/src/utils/stats/SurveyStatsCalculator.test.ts(1 hunks)shared/src/utils/stats/SurveyStatsCalculator.ts(1 hunks)ui/src/components/badge.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/survey/responses/barchart-survey-response-component.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/survey/responses/card-tab.css(1 hunks)website/src/app/[lang]/[region]/(website)/survey/responses/opacity.css(1 hunks)website/src/app/[lang]/[region]/(website)/survey/responses/page.tsx(1 hunks)website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questionnaires.ts(4 hunks)website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questions.ts(21 hunks)
✅ Files skipped from review due to trivial changes (1)
- website/src/app/[lang]/[region]/(website)/survey/responses/card-tab.css
🚧 Files skipped from review as they are similar to previous changes (10)
- website/src/app/[lang]/[region]/(website)/survey/responses/opacity.css
- ui/src/components/badge.tsx
- shared/locales/en/website-survey.json
- shared/locales/en/website-responses.json
- shared/src/utils/stats/SurveyStatsCalculator.test.ts
- shared/locales/it/website-survey.json
- shared/locales/kri/website-survey.json
- website/src/app/[lang]/[region]/(website)/survey/responses/page.tsx
- website/src/app/[lang]/[region]/(website)/survey/responses/barchart-survey-response-component.tsx
- website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questionnaires.ts
🧰 Additional context used
🪛 Biome (1.9.4)
website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questions.ts
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 60-60: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
[error] 350-350: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
shared/src/utils/stats/SurveyStatsCalculator.ts
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 72-72: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 80-80: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🪛 Gitleaks (8.21.2)
shared/src/types/question.ts
47-47: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
235-235: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (8)
shared/src/types/question.ts (4)
1-8: Improve type safety for Question interface
The choices property using any[] reduces type safety. Consider using a more specific type.
10-15: LGTM!
The enum is well-defined with clear, descriptive values.
230-236: Add choices for RANKING question type
The RANKING question type requires choices to be ranked, but the choices array is empty.
🧰 Tools
🪛 Gitleaks (8.21.2)
235-235: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
271-271: LGTM!
The dictionary implementation is correct and uses proper TypeScript types.
shared/src/utils/stats/SurveyStatsCalculator.ts (2)
4-4:
Fix typo in constant name
SURVEY_FIRETORE_PATH should be SURVEY_FIRESTORE_PATH.
-import { Survey, SURVEY_FIRETORE_PATH, SurveyQuestionnaire, SurveyStatus } from '../../types/survey';
+import { Survey, SURVEY_FIRESTORE_PATH, SurveyQuestionnaire, SurveyStatus } from '../../types/survey';Likely invalid or redundant comment.
92-97: 🛠️ Refactor suggestion
Improve type safety for survey response
The response parameter should have a more specific type than any.
private static processSurveyResponse(
aggregatedData: { [key: string]: { [key: string]: SurveyAnswersByType } },
questionnaire: string,
questionKey: string,
- response: any,
+ response: string | string[],
): void {Likely invalid or redundant comment.
website/src/app/[lang]/[region]/survey/[recipient]/[survey]/questions.ts (2)
350-354: 🛠️ Refactor suggestion
Improve type safety in translateChoices
The function uses any[] and uppercase String type.
-export const translateChoices = (t: TranslateFunction, choices: any[], choicesTranslationKey: String) =>
+export const translateChoices = (t: TranslateFunction, choices: string[], choicesTranslationKey: string): Array<{ value: string; text: string }> =>
choices.map((key) => ({
value: key,
text: t(choicesTranslationKey + '.' + key),
}));Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 350-350: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
60-71: 🛠️ Refactor suggestion
Improve type safety and null handling in getSimpleMapping
The function needs proper return type definition and better null handling.
+interface QuestionMapping {
+ type: string;
+ name: string;
+ title: string;
+ description?: string;
+ choices?: Array<{ value: string; text: string }>;
+}
-function getSimpleMapping(question: Question, t: TranslateFunction): Object {
+function getSimpleMapping(question: Question, t: TranslateFunction): QuestionMapping {
return {
type: question.type,
name: question.name,
title: t(question.translationKey),
- description: question.descriptionTranslationKey && t(question.descriptionTranslationKey),
+ description: question.descriptionTranslationKey ? t(question.descriptionTranslationKey) : undefined,
choices:
- question.choices && question.choices.length
+ question.choices?.length
? translateChoices(t, question.choices, question.choicesTranslationKey!)
: undefined,
};
}Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 60-60: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
website/src/app/[lang]/[region]/(website)/survey/responses/barchart-survey-response-component.tsx (1)
9-14: Use const for fillColorThe fillColor variable never gets reassigned.
-let fillColor = 'hsl(var(--foreground))'; +const fillColor = 'hsl(var(--foreground))';shared/src/utils/stats/SurveyStatsCalculator.ts (1)
124-130: Add type guards for response handlingConsider adding type guards instead of type casting for better type safety.
-if (question.type === QuestionInputType.CHECKBOX) { - (response as string[]).forEach((value) => { +if (question.type === QuestionInputType.CHECKBOX && Array.isArray(response)) { + response.forEach((value: string) => { questionData.answers[value] = (questionData.answers[value] || 0) + 1; }); -} else if (question.type === QuestionInputType.RADIO_GROUP) { - const responseValue = response as string; +} else if (question.type === QuestionInputType.RADIO_GROUP && typeof response === 'string') { - questionData.answers[responseValue] = (questionData.answers[responseValue] || 0) + 1; + questionData.answers[response] = (questionData.answers[response] || 0) + 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
shared/src/utils/stats/SurveyStatsCalculator.test.ts(1 hunks)shared/src/utils/stats/SurveyStatsCalculator.ts(1 hunks)website/src/app/[lang]/[region]/(website)/survey/responses/barchart-survey-response-component.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/survey/responses/page.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- shared/src/utils/stats/SurveyStatsCalculator.test.ts
- website/src/app/[lang]/[region]/(website)/survey/responses/page.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
shared/src/utils/stats/SurveyStatsCalculator.ts
[error] 50-50: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 51-51: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 87-87: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 95-95: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (6)
website/src/app/[lang]/[region]/(website)/survey/responses/barchart-survey-response-component.tsx (3)
1-7: LGTM! Clean imports and interface definition.
15-30: Type the label props properly
Replace the any type with a proper interface for better type safety.
32-59: Well-implemented chart with accessibility considerations!
The chart implementation includes proper ARIA labels and responsive layout.
shared/src/utils/stats/SurveyStatsCalculator.ts (3)
4-4:
Fix typo in import path constant
The constant SURVEY_FIRETORE_PATH is misspelled. It should be SURVEY_FIRESTORE_PATH.
-import { Survey, SURVEY_FIRETORE_PATH, SurveyQuestionnaire, SurveyStatus } from '../../types/survey';
+import { Survey, SURVEY_FIRESTORE_PATH, SurveyQuestionnaire, SurveyStatus } from '../../types/survey';Likely invalid or redundant comment.
50-51: 🛠️ Refactor suggestion
Replace 'this' with class name in static methods
Using 'this' in static methods can be confusing. Use the class name instead.
-const surveysData = await this.fetchAndProcessSurveys(firestoreAdmin, recipients);
-const { typeCounts, aggregatedData, oldestDate } = this.aggregateSurveyData(surveysData);
+const surveysData = await SurveyStatsCalculator.fetchAndProcessSurveys(firestoreAdmin, recipients);
+const { typeCounts, aggregatedData, oldestDate } = SurveyStatsCalculator.aggregateSurveyData(surveysData);Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 50-50: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 51-51: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
60-68:
Add error handling for Promise.all
The Promise.all operation could fail if any of the promises reject.
+try {
const surveySnapshots = await Promise.all(
recipients.docs
.filter((recipient) => !recipient.get('test_recipient'))
.map((recipient) =>
firestoreAdmin
.collection<Survey>(`${RECIPIENT_FIRESTORE_PATH}/${recipient.id}/${SURVEY_FIRETORE_PATH}`)
.get(),
),
);
+} catch (error) {
+ console.error('Failed to fetch survey snapshots:', error);
+ throw error;
+}Likely invalid or redundant comment.
| let oldestDate = new Date(); | ||
|
|
||
| surveysData.forEach((survey) => { | ||
| if (this.isCompletedSurvey(survey)) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and static method calls
- Replace 'this' with class name in static methods
- Add type safety for survey response handling
-if (this.isCompletedSurvey(survey)) {
+if (SurveyStatsCalculator.isCompletedSurvey(survey)) {
Object.entries(survey.data!).forEach(([questionKey, response]) => {
- this.processSurveyResponse(aggregatedSurveyData, questionnaire, questionKey, response);
+ SurveyStatsCalculator.processSurveyResponse(aggregatedSurveyData, questionnaire, questionKey, response);
});Also applies to: 94-96
🧰 Tools
🪛 Biome (1.9.4)
[error] 87-87: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
shared/src/utils/stats/SurveyStatsCalculator.ts (2)
4-4:⚠️ Potential issueFix typo in import path constant
The constant
SURVEY_FIRETORE_PATHis misspelled. It should beSURVEY_FIRESTORE_PATH.
50-51: 🛠️ Refactor suggestionReplace 'this' with class name in static methods
Using 'this' in static methods can be confusing. Use the class name instead.
🧰 Tools
🪛 Biome (1.9.4)
[error] 50-50: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 51-51: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
shared/src/utils/stats/SurveyStatsCalculator.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
shared/src/utils/stats/SurveyStatsCalculator.ts
[error] 50-50: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 51-51: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 87-87: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 95-95: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (2)
shared/src/utils/stats/SurveyStatsCalculator.ts (2)
21-42: LGTM! Well-structured class implementation
The class follows good practices with immutable fields, private constructor, and clear getter methods.
111-111: 🛠️ Refactor suggestion
Replace 'any' with a specific type
Using 'any' reduces type safety. Define a union type for possible response values.
- response: any,
+ response: string | string[],Likely invalid or redundant comment.
| if (question.type === QuestionInputType.CHECKBOX) { | ||
| (response as string[]).forEach((value) => { | ||
| questionData.answers[value] = (questionData.answers[value] || 0) + 1; | ||
| }); | ||
| } else if (question.type === QuestionInputType.RADIO_GROUP) { | ||
| const responseValue = response as string; | ||
| questionData.answers[responseValue] = (questionData.answers[responseValue] || 0) + 1; | ||
| } | ||
| questionData.total++; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for response data structure
The response data should be validated before processing to prevent runtime errors.
if (question.type === QuestionInputType.CHECKBOX) {
+ if (!Array.isArray(response)) {
+ console.warn(`Invalid checkbox response for question ${questionKey}`);
+ return;
+ }
(response as string[]).forEach((value) => {
questionData.answers[value] = (questionData.answers[value] || 0) + 1;
});
} else if (question.type === QuestionInputType.RADIO_GROUP) {
+ if (typeof response !== 'string') {
+ console.warn(`Invalid radio response for question ${questionKey}`);
+ return;
+ }
const responseValue = response as string;
questionData.answers[responseValue] = (questionData.answers[responseValue] || 0) + 1;
}📝 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.
| if (question.type === QuestionInputType.CHECKBOX) { | |
| (response as string[]).forEach((value) => { | |
| questionData.answers[value] = (questionData.answers[value] || 0) + 1; | |
| }); | |
| } else if (question.type === QuestionInputType.RADIO_GROUP) { | |
| const responseValue = response as string; | |
| questionData.answers[responseValue] = (questionData.answers[responseValue] || 0) + 1; | |
| } | |
| questionData.total++; | |
| } | |
| if (question.type === QuestionInputType.CHECKBOX) { | |
| if (!Array.isArray(response)) { | |
| console.warn(`Invalid checkbox response for question ${questionKey}`); | |
| return; | |
| } | |
| (response as string[]).forEach((value) => { | |
| questionData.answers[value] = (questionData.answers[value] || 0) + 1; | |
| }); | |
| } else if (question.type === QuestionInputType.RADIO_GROUP) { | |
| if (typeof response !== 'string') { | |
| console.warn(`Invalid radio response for question ${questionKey}`); | |
| return; | |
| } | |
| const responseValue = response as string; | |
| questionData.answers[responseValue] = (questionData.answers[responseValue] || 0) + 1; | |
| } | |
| questionData.total++; | |
| } |
| static async build(firestoreAdmin: FirestoreAdmin): Promise<SurveyStatsCalculator> { | ||
| const recipients = await firestoreAdmin.collection<Recipient>(RECIPIENT_FIRESTORE_PATH).get(); | ||
| const surveysData = await this.fetchAndProcessSurveys(firestoreAdmin, recipients); | ||
| const { typeCounts, aggregatedData, oldestDate } = this.aggregateSurveyData(surveysData); | ||
| const data = Object.entries(typeCounts).map(([type, total]) => ({ type, total }) as SurveyStats); | ||
| return new SurveyStatsCalculator(data, aggregatedData, oldestDate); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for database operations
The build method should handle potential database errors gracefully.
static async build(firestoreAdmin: FirestoreAdmin): Promise<SurveyStatsCalculator> {
+ try {
const recipients = await firestoreAdmin.collection<Recipient>(RECIPIENT_FIRESTORE_PATH).get();
const surveysData = await SurveyStatsCalculator.fetchAndProcessSurveys(firestoreAdmin, recipients);
const { typeCounts, aggregatedData, oldestDate } = SurveyStatsCalculator.aggregateSurveyData(surveysData);
const data = Object.entries(typeCounts).map(([type, total]) => ({ type, total }) as SurveyStats);
return new SurveyStatsCalculator(data, aggregatedData, oldestDate);
+ } catch (error) {
+ console.error('Failed to build SurveyStatsCalculator:', error);
+ throw 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.
| static async build(firestoreAdmin: FirestoreAdmin): Promise<SurveyStatsCalculator> { | |
| const recipients = await firestoreAdmin.collection<Recipient>(RECIPIENT_FIRESTORE_PATH).get(); | |
| const surveysData = await this.fetchAndProcessSurveys(firestoreAdmin, recipients); | |
| const { typeCounts, aggregatedData, oldestDate } = this.aggregateSurveyData(surveysData); | |
| const data = Object.entries(typeCounts).map(([type, total]) => ({ type, total }) as SurveyStats); | |
| return new SurveyStatsCalculator(data, aggregatedData, oldestDate); | |
| } | |
| static async build(firestoreAdmin: FirestoreAdmin): Promise<SurveyStatsCalculator> { | |
| try { | |
| const recipients = await firestoreAdmin.collection<Recipient>(RECIPIENT_FIRESTORE_PATH).get(); | |
| const surveysData = await SurveyStatsCalculator.fetchAndProcessSurveys(firestoreAdmin, recipients); | |
| const { typeCounts, aggregatedData, oldestDate } = SurveyStatsCalculator.aggregateSurveyData(surveysData); | |
| const data = Object.entries(typeCounts).map(([type, total]) => ({ type, total }) as SurveyStats); | |
| return new SurveyStatsCalculator(data, aggregatedData, oldestDate); | |
| } catch (error) { | |
| console.error('Failed to build SurveyStatsCalculator:', error); | |
| throw error; | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 50-50: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 51-51: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
There was a problem hiding this comment.
Super nice work, thanks @Gavriil-Tzortzakis :)
The only thing that I would discourage from doing is importing .css files. I feel that with Tailwind, all styling should be done inline. The styling is imo more transparent like that – even when sometimes elements have a lot of classes specified.
I added one commit where I removed the .css files and added the styling as tailwind classes instead. Also, some other small cleanups (mostly typing), but those are just nits.
I hope it's fine if I do this – instead of commenting everywhere. I feel it's a little more efficient, but please let me know if you prefer comments.
Here's the commit with my changes: 6abd527
...te/src/app/[lang]/[region]/(website)/survey/responses/barchart-survey-response-component.tsx
Show resolved
Hide resolved
| @@ -0,0 +1,271 @@ | |||
| export interface Question { | |||
There was a problem hiding this comment.
Very nice! Thanks for improving the types here.
Summary by CodeRabbit
Release Notes
New Features
Localization
UI Improvements
Technical Enhancements