feat: enabled viewing historical finance data #3658
Conversation
SahilDahekar
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThis pull request implements dynamic year filtering for finance data visualization. It introduces new state variables (mounted, selectedYear, and currentData) in several components, enabling conditional rendering after mounting. Data is now fetched dynamically using a new utility function and is selectable by year, with an option to view data for all years. The changes update the BarChartComponent, ExpensesCard, and Card components to support dynamic expense and link data based on the selected year. Additionally, related utility functions and scripts have been refactored to aggregate multi-year data. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant BC as BarChartComponent
participant DL as loadYearData
participant EC as ExpensesCard
participant C as Card
U->>BC: Selects a year from dropdown
BC->>DL: Call loadYearData(selectedYear)
DL-->>BC: Return expensesData & expensesLinkData
BC->>BC: Update currentData based on selectedYear
BC->>EC: Pass selectedYear and data as props
EC->>C: Forward expensesLinkData and monthly data
C-->>EC: Render updated expense cards
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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
Documentation and Community
|
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
These lint error are shown but i am not able to understand how should i fix them . The lines where this error is shown is a useEffect function that i changed to resolve the hydration issue.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
config/finance/2024/Expenses.json (1)
247-247: Remove Potential Stray ContentLine 247 appears to contain extraneous content ("247") that may have been introduced inadvertently. If this is not intended to be part of the JSON content, please remove it to ensure the file remains valid JSON. For example, applying the diff below would remove the stray line:
-247
🧹 Nitpick comments (4)
components/FinancialSummary/BarChartComponent.tsx (1)
59-109: Simplify year data loading logic.The current implementation of year data loading is complex and could be simplified.
Consider extracting the data loading logic into a separate function:
const loadAllYearsData = () => { const allYearsData = { ...ExpensesData }; const allLinksData = [...ExpensesLinkData]; years.forEach((year) => { const { expensesData, expensesLinkData } = loadYearData(year); if (Object.keys(expensesData).length > 0) { Object.entries(expensesData).forEach(([month, entries]) => { allYearsData[month] = allYearsData[month] || []; allYearsData[month].push(...entries); }); expensesLinkData.forEach((link) => { if (!allLinksData.some((l) => l.category === link.category)) { allLinksData.push(link); } }); } }); return { expensesData: allYearsData, expensesLinkData: allLinksData }; };config/finance/2023/Expenses.json (1)
1-194: Standardize amount formatting and consolidate duplicate entries.The JSON file has inconsistent number formatting and contains duplicate categories within the same month that could be consolidated.
Consider:
- Standardizing all amounts to have 2 decimal places
- Consolidating duplicate categories within the same month
Example for January:
{ "January": [ { "Category": "Ambassador Program", - "Amount": "68.95" + "Amount": "68.95" }, { "Category": "Google Season of Docs 2022", - "Amount": "35.62" + "Amount": "1702.29" - }, - { - "Category": "Google Season of Docs 2022", - "Amount": "1666.67" }, { "Category": "AsyncAPI Mentorship 2022", - "Amount": "1500" + "Amount": "4500.00" - }, - { - "Category": "AsyncAPI Mentorship 2022", - "Amount": "1500" - }, - { - "Category": "AsyncAPI Mentorship 2022", - "Amount": "1500" } ],config/finance/2024/Expenses.json (2)
1-246: JSON Structure and Data Type ConsiderationThe JSON file is well-structured with clear month keys and arrays of expense entries. Each expense entry consistently includes a "Category" and an "Amount." One suggestion: if these "Amount" values are later used in arithmetic operations, consider storing them as numeric values instead of strings (or ensure that any consuming code performs the appropriate conversion).
1-247: Verify Consistency in Category NamingSome entries reference specific event years—for example, an expense in January is labeled as "AsyncAPI Conf on Tour 2023" while later entries (e.g., in July and October) specify "AsyncAPI Conf on Tour 2024." Please verify that these naming conventions are intentional and that they accurately reflect the events for the 2024 finance context, or update them for consistency if needed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
components/FinancialSummary/BarChartComponent.tsx(5 hunks)config/finance/2023/Expenses.json(1 hunks)config/finance/2023/ExpensesLink.json(1 hunks)config/finance/2024/Expenses.json(1 hunks)config/finance/2024/ExpensesLink.json(1 hunks)pages/finance.tsx(2 hunks)utils/loadYearData.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- config/finance/2023/ExpensesLink.json
- config/finance/2024/ExpensesLink.json
🧰 Additional context used
🪛 ESLint
utils/loadYearData.ts
[error] 3-3: Calls to require() should use string literals
(import/no-dynamic-require)
[error] 3-3: Unexpected require().
(global-require)
[error] 4-4: Expected blank line after variable declarations.
(newline-after-var)
[error] 4-4: Calls to require() should use string literals
(import/no-dynamic-require)
[error] 4-4: Unexpected require().
(global-require)
[error] 5-5: Expected blank line before this statement.
(padding-line-between-statements)
[error] 8-8: Expected blank line before this statement.
(padding-line-between-statements)
pages/finance.tsx
[error] 25-25: Expected to return a value at the end of arrow function.
(consistent-return)
components/FinancialSummary/BarChartComponent.tsx
[error] 43-43: Expected to return a value at the end of arrow function.
(consistent-return)
🪛 GitHub Actions: PR testing - if Node project
pages/finance.tsx
[error] 25-25: Expected to return a value at the end of arrow function. consistent-return
components/FinancialSummary/BarChartComponent.tsx
[error] 43-43: Expected to return a value at the end of arrow function. consistent-return
⏰ Context from checks skipped due to timeout of 180000ms (4)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: Lighthouse CI
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3658 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 20 20
Lines 732 760 +28
=========================================
+ Hits 732 760 +28 ☔ View full report in Codecov by Sentry. |
This is resolved now |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
utils/loadYearData.ts (2)
12-21: Remove unnecessary quotes from year properties.The year properties in
YEAR_DATA_MAPdon't need to be quoted as they are valid identifiers.const YEAR_DATA_MAP: { [key: string]: YearData } = { - '2023': { + 2023: { expenses: expenses2023, links: expensesLink2023 }, - '2024': { + 2024: { expenses: expenses2024, links: expensesLink2024 } };🧰 Tools
🪛 ESLint
[error] 13-16: Unnecessarily quoted property '2023' found.
(quote-props)
[error] 17-20: Unnecessarily quoted property '2024' found.
(quote-props)
23-35: Simplify return type using YearData interface.The return type can be simplified by leveraging the
YearDatainterface with renamed properties.-export const loadYearData = (year: string): { expensesData: { [key: string]: ExpenseItem[] }, expensesLinkData: ExpensesLinkItem[] } => { +export const loadYearData = (year: string): { expensesData: YearData['expenses'], expensesLinkData: YearData['links'] } => {🧰 Tools
🪛 ESLint
[error] 23-23: Replace
year:·string):·{·expensesData:·{·[key:·string]:·ExpenseItem[]·},with⏎··year:·string⏎):·{·expensesData:·{·[key:·string]:·ExpenseItem[]·};(prettier/prettier)
[error] 25-25: Expected blank line after variable declarations.
(newline-after-var)
[error] 26-28: Expected blank line before this statement.
(padding-line-between-statements)
[error] 29-29: Expected blank line after variable declarations.
(newline-after-var)
[error] 30-30: Expected blank line before this statement.
(padding-line-between-statements)
[error] 33-33: Expected blank line before this statement.
(padding-line-between-statements)
[error] 35-35: Newline required at end of file but not found.
(eol-last)
[error] 35-35: Insert
⏎(prettier/prettier)
components/FinancialSummary/BarChartComponent.tsx (1)
56-106: Add loading state and simplify data loading logic.The data loading logic could benefit from:
- Adding a loading state while data is being fetched
- Simplifying the 'All Years' data aggregation logic
+ const [isLoading, setIsLoading] = useState(false); useEffect(() => { + setIsLoading(true); if (selectedYear === 'All Years') { - const allYearsData: { [key: string]: ExpenseItem[] } = {}; - const allLinksData: ExpensesLinkItem[] = [...ExpensesLinkData]; - - // Start with the default JSON data - Object.keys(ExpensesData).forEach((month) => { - allYearsData[month] = [...ExpensesData[month as keyof typeof ExpensesData]]; - }); + const allYearsData = { ...ExpensesData }; + const allLinksData = new Set(ExpensesLinkData); years.forEach((year) => { const { expensesData, expensesLinkData } = loadYearData(year); if (Object.keys(expensesData).length > 0) { Object.entries(expensesData).forEach(([month, entries]) => { - if (!allYearsData[month]) { - allYearsData[month] = []; - } - allYearsData[month].push(...expensesData[month]); + allYearsData[month] = [...(allYearsData[month] || []), ...entries]; }); - expensesLinkData.forEach((link: ExpensesLinkItem) => { - if (!allLinksData.some((l) => l.category === link.category)) { - allLinksData.push(link); - } - }); + expensesLinkData.forEach(link => allLinksData.add(link)); } }); setCurrentData({ expensesData: allYearsData, - expensesLinkData: allLinksData + expensesLinkData: Array.from(allLinksData) }); } else { const { expensesData, expensesLinkData } = loadYearData(selectedYear); setCurrentData( Object.keys(expensesData).length === 0 ? { expensesData: ExpensesData, expensesLinkData: ExpensesLinkData } : { expensesData, expensesLinkData } ); } + setIsLoading(false); }, [selectedYear]); + if (isLoading) { + return <div>Loading...</div>; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/FinancialSummary/BarChartComponent.tsx(5 hunks)pages/finance.tsx(2 hunks)utils/loadYearData.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
utils/loadYearData.ts
[error] 1-5: Run autofix to sort these imports!
(simple-import-sort/imports)
[error] 13-16: Unnecessarily quoted property '2023' found.
(quote-props)
[error] 17-20: Unnecessarily quoted property '2024' found.
(quote-props)
[error] 23-23: Replace year:·string):·{·expensesData:·{·[key:·string]:·ExpenseItem[]·}, with ⏎··year:·string⏎):·{·expensesData:·{·[key:·string]:·ExpenseItem[]·};
(prettier/prettier)
[error] 25-25: Expected blank line after variable declarations.
(newline-after-var)
[error] 26-28: Expected blank line before this statement.
(padding-line-between-statements)
[error] 29-29: Expected blank line after variable declarations.
(newline-after-var)
[error] 30-30: Expected blank line before this statement.
(padding-line-between-statements)
[error] 33-33: Expected blank line before this statement.
(padding-line-between-statements)
[error] 35-35: Newline required at end of file but not found.
(eol-last)
[error] 35-35: Insert ⏎
(prettier/prettier)
🔇 Additional comments (2)
pages/finance.tsx (1)
24-37: Fix useEffect return type and extract window resize logic.The useEffect hook needs a proper return type, and the window resize logic could be extracted into a custom hook for better reusability.
- useEffect(() => { + useEffect((): (() => void) | void => { setMounted(true); if (typeof window !== 'undefined') { setWindowWidth(window.innerWidth); const handleResize = () => { setWindowWidth(window.innerWidth); }; window.addEventListener('resize', handleResize); return () => window.removeEventListener('resize', handleResize); } }, []);Consider extracting the window resize logic into a custom hook:
function useWindowWidth() { const [windowWidth, setWindowWidth] = useState(0); useEffect((): (() => void) | void => { if (typeof window !== 'undefined') { const handleResize = () => setWindowWidth(window.innerWidth); handleResize(); window.addEventListener('resize', handleResize); return () => window.removeEventListener('resize', handleResize); } }, []); return windowWidth; }components/FinancialSummary/BarChartComponent.tsx (1)
38-53: Fix useEffect return type.The useEffect hook needs a proper return type to satisfy ESLint.
- useEffect(() => { + useEffect((): (() => void) | void => { setMounted(true); if (typeof window !== 'undefined') { const handleResize = () => { setWindowWidth(window.innerWidth); }; handleResize(); window.addEventListener('resize', handleResize); return () => { window.removeEventListener('resize', handleResize); }; } }, []);
SahilDahekar
changed the title
|
@SahilDahekar cards in the mobile view still display the same data for every year, which should not be happening. The data should change according to the selected year. Also, you have added config data in this PR, but I think you can remove it since we already have it. |
|
It is behaving similar to what the current behaviour is . For the year i think i do need to make some changes though .Do let me know if you find any other issues @vishvamsinh28 |
|
@SahilDahekar updated my previous comment please have a look at it and you can click on deploy preview (mobile view) and change the year you will notice that the data in the cards remains same |
|
For the config data yml format is used in other places and for the charts i need json format of the data so they had to be added . What are your thoughts ? Also i think we should have a seperate config for the combined year data |
|
@SahilDahekar We have a script that converts the YAML file into JSON data, so you don't need to do it manually or add it to the PR. When you run npm run dev, JSON files are generated for the finance data (these files are also added to .gitignore, so they don’t get pushed with the PR) https://github.com/asyncapi/website/blob/master/scripts/finance/index.js |
|
Okay thanks for sharing this @vishvamsinh28 . Will make the necessary changes soon |
|
Is there any other way to handle YAML, apart from converting it to JSON? |
|
@Chayan8837 We are handling this with YAML, and the JSON part is automated. When we make changes to the YAML file, the JSON files get updated automatically. Apart from that, I don’t think this can be done with YAML alone; we will need JSON for it |
|
Hey @JeelRajodiya you updated the finance script for automatic year detection, but I think there's an issue. Can you check it locally once? When I added data for 2025, it worked as expected, but when I checked the JSON data folder, the previous year's data was not structured properly, and the data for 2023 was missing. @SahilDahekar We want the JSON data to be structured the same way as the YAML files, organized year-wise under json-data folder |
… of manually adding files
|
@vishvamsinh28 check the correct push once i changed the script to generate json data for each year including one for all years combined |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
components/FinancialSummary/ExpensesCard.tsx (1)
11-11: Consider adding validation for the year prop.While the type is correctly defined, consider adding validation to ensure the year is in a valid format and range.
-export default function ExpensesCard({ year }: { year: string }) { +export default function ExpensesCard({ year }: { year: string }) { + if (!/^\d{4}$/.test(year) && year !== 'All Years') { + throw new Error('Invalid year format. Expected a 4-digit year or "All Years".'); + }components/FinancialSummary/BarChartComponent.tsx (3)
1-10: Consider using dynamic imports for JSON data.The static JSON imports could be replaced with dynamic imports to improve initial load time.
-import ExpensesData from '../../config/finance/json-data/2024/Expenses.json'; -import ExpensesLinkData from '../../config/finance/json-data/2024/ExpensesLink.json'; +const DEFAULT_YEAR = '2024'; + +async function getDefaultData() { + const [ExpensesData, ExpensesLinkData] = await Promise.all([ + import(`../../config/finance/json-data/${DEFAULT_YEAR}/Expenses.json`), + import(`../../config/finance/json-data/${DEFAULT_YEAR}/ExpensesLink.json`) + ]); + return { ExpensesData: ExpensesData.default, ExpensesLinkData: ExpensesLinkData.default }; +}
71-81: Optimize performance with memoization.Consider memoizing the filtered data to prevent unnecessary recalculations.
-const months: string[] = Object.keys(currentData.expensesData); +const months = useMemo(() => Object.keys(currentData.expensesData), [currentData.expensesData]); -const filteredData: ExpenseItem[] = Object.entries(currentData.expensesData).flatMap(([month, entries]) => +const filteredData = useMemo(() => Object.entries(currentData.expensesData).flatMap(([month, entries]) => selectedMonth === 'All Months' || selectedMonth === month ? entries.filter( (entry: ExpenseItem) => selectedCategory === 'All Categories' || entry.Category === selectedCategory ) : [] -); +), [currentData.expensesData, selectedMonth, selectedCategory]);
150-161: Enhance accessibility for year selection.Add ARIA labels and improve keyboard navigation for the year selection dropdown.
<select className='m-1 w-full rounded-md border border-gray-600 bg-white p-2 text-xs font-semibold text-violet sm:w-auto md:w-48' value={selectedYear} onChange={(e) => setSelectedYear(e.target.value)} + aria-label="Select year" + role="combobox" >Also, consider adding a loading state for the ExpensesCard:
-{windowWidth && windowWidth < 900 ? <ExpensesCard year={selectedYear} /> : null} +{windowWidth && windowWidth < 900 ? ( + <Suspense fallback={<div>Loading expenses...</div>}> + <ExpensesCard year={selectedYear} /> + </Suspense> +) : null}Also applies to: 188-188
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/FinancialSummary/BarChartComponent.tsx(6 hunks)components/FinancialSummary/Card.tsx(1 hunks)components/FinancialSummary/ExpensesCard.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/FinancialSummary/Card.tsx
🔇 Additional comments (1)
components/FinancialSummary/ExpensesCard.tsx (1)
4-4: LGTM!The import of
loadYearDatautility is correctly added to support loading year-specific data.
| const { expensesData, expensesLinkData } = loadYearData(year); | ||
|
|
||
| return ( | ||
| <div className='overflow-x-auto'> | ||
| <div className='grid auto-cols-max grid-flow-col gap-4 p-4'> | ||
| {Object.entries(ExpensesData).map(function ([month, data], index) { | ||
| return <Card key={index} month={month as keyof Expenses} data={data} />; | ||
| {Object.entries(expensesData).map(function ([month, data], index) { | ||
| return <Card key={index} month={month as keyof Expenses} data={data} expensesLinkData={expensesLinkData} />; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for data loading.
Consider handling potential errors when loading year data and provide a fallback UI.
- const { expensesData, expensesLinkData } = loadYearData(year);
+ const [error, setError] = useState<Error | null>(null);
+ const [data, setData] = useState<ReturnType<typeof loadYearData> | null>(null);
+
+ useEffect(() => {
+ try {
+ const yearData = loadYearData(year);
+ setData(yearData);
+ setError(null);
+ } catch (err) {
+ setError(err instanceof Error ? err : new Error('Failed to load data'));
+ setData(null);
+ }
+ }, [year]);
+
+ if (error) {
+ return <div className="text-red-500">Error loading data: {error.message}</div>;
+ }
+
+ if (!data) {
+ return <div>Loading...</div>;
+ }
+
+ const { expensesData, expensesLinkData } = data;📝 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 { expensesData, expensesLinkData } = loadYearData(year); | |
| return ( | |
| <div className='overflow-x-auto'> | |
| <div className='grid auto-cols-max grid-flow-col gap-4 p-4'> | |
| {Object.entries(ExpensesData).map(function ([month, data], index) { | |
| return <Card key={index} month={month as keyof Expenses} data={data} />; | |
| {Object.entries(expensesData).map(function ([month, data], index) { | |
| return <Card key={index} month={month as keyof Expenses} data={data} expensesLinkData={expensesLinkData} />; | |
| const [error, setError] = useState<Error | null>(null); | |
| const [data, setData] = useState<ReturnType<typeof loadYearData> | null>(null); | |
| useEffect(() => { | |
| try { | |
| const yearData = loadYearData(year); | |
| setData(yearData); | |
| setError(null); | |
| } catch (err) { | |
| setError(err instanceof Error ? err : new Error('Failed to load data')); | |
| setData(null); | |
| } | |
| }, [year]); | |
| if (error) { | |
| return <div className="text-red-500">Error loading data: {error.message}</div>; | |
| } | |
| if (!data) { | |
| return <div>Loading...</div>; | |
| } | |
| const { expensesData, expensesLinkData } = data; | |
| return ( | |
| <div className='overflow-x-auto'> | |
| <div className='grid auto-cols-max grid-flow-col gap-4 p-4'> | |
| {Object.entries(expensesData).map(function ([month, data], index) { | |
| return <Card key={index} month={month as keyof Expenses} data={data} expensesLinkData={expensesLinkData} />; | |
| })} | |
| </div> | |
| </div> | |
| ); |
| useEffect(() => { | ||
| // Load data for the selected year (or All_years) | ||
| const { expensesData, expensesLinkData } = loadYearData(selectedYear === 'All Years' ? 'All Years' : selectedYear); | ||
|
|
||
| if (Object.keys(expensesData).length === 0) { | ||
| // If no data found, fallback to default data | ||
| setCurrentData({ | ||
| expensesData: ExpensesData, | ||
| expensesLinkData: ExpensesLinkData | ||
| }); | ||
| } else { | ||
| setCurrentData({ expensesData, expensesLinkData }); | ||
| } | ||
| }, [selectedYear]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to year data loading effect.
The year data loading effect should handle potential errors and provide user feedback.
useEffect(() => {
+ const [error, setError] = useState<Error | null>(null);
+ setError(null);
+ try {
const { expensesData, expensesLinkData } = loadYearData(selectedYear === 'All Years' ? 'All Years' : selectedYear);
if (Object.keys(expensesData).length === 0) {
setCurrentData({
expensesData: ExpensesData,
expensesLinkData: ExpensesLinkData
});
} else {
setCurrentData({ expensesData, expensesLinkData });
}
+ } catch (err) {
+ setError(err instanceof Error ? err : new Error('Failed to load data'));
+ // Fallback to default data
+ setCurrentData({
+ expensesData: ExpensesData,
+ expensesLinkData: ExpensesLinkData
+ });
+ }
}, [selectedYear]);
+
+if (error) {
+ // Show error toast or notification
+ console.error('Error loading year data:', error);
+}Committable suggestion skipped: line range outside the PR's diff.
| const [mounted, setMounted] = useState(false); | ||
| // Setting up state variables using useState hook | ||
| const [selectedCategory, setSelectedCategory] = useState<string>('All Categories'); | ||
| const [selectedMonth, setSelectedMonth] = useState<string>('All Months'); | ||
| const [selectedYear, setSelectedYear] = useState<string>('2024'); | ||
| const [windowWidth, setWindowWidth] = useState<number>(0); | ||
| const [currentData, setCurrentData] = useState<{ | ||
| expensesData: { [key: string]: ExpenseItem[] }; | ||
| expensesLinkData: ExpensesLinkItem[]; | ||
| }>({ | ||
| expensesData: ExpensesData, // Use JSON data as initial value | ||
| expensesLinkData: ExpensesLinkData | ||
| }); | ||
|
|
||
| // Extracting unique categories and months from the data | ||
| const categories: string[] = getUniqueCategories(); | ||
| const months: string[] = Object.keys(ExpensesData); | ||
| // Extracting unique categories from the data | ||
| const categories: string[] = getUniqueCategories({ selectedYear }); | ||
| const years: string[] = ['2023', '2024']; // Add more years as needed |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make years array dynamic.
Instead of hardcoding the years array, consider generating it dynamically based on available data.
-const years: string[] = ['2023', '2024']; // Add more years as needed
+const years: string[] = useMemo(() => {
+ try {
+ // Get all available year folders
+ const availableYears = Object.keys(require.context(
+ '../../config/finance/json-data',
+ true,
+ /\d{4}\/Expenses\.json$/
+ )).map(path => path.match(/(\d{4})/)?.[1] ?? '');
+
+ return [...new Set(availableYears)].sort().reverse();
+ } catch {
+ return ['2023', '2024']; // Fallback to default years
+ }
+}, []);📝 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 [mounted, setMounted] = useState(false); | |
| // Setting up state variables using useState hook | |
| const [selectedCategory, setSelectedCategory] = useState<string>('All Categories'); | |
| const [selectedMonth, setSelectedMonth] = useState<string>('All Months'); | |
| const [selectedYear, setSelectedYear] = useState<string>('2024'); | |
| const [windowWidth, setWindowWidth] = useState<number>(0); | |
| const [currentData, setCurrentData] = useState<{ | |
| expensesData: { [key: string]: ExpenseItem[] }; | |
| expensesLinkData: ExpensesLinkItem[]; | |
| }>({ | |
| expensesData: ExpensesData, // Use JSON data as initial value | |
| expensesLinkData: ExpensesLinkData | |
| }); | |
| // Extracting unique categories and months from the data | |
| const categories: string[] = getUniqueCategories(); | |
| const months: string[] = Object.keys(ExpensesData); | |
| // Extracting unique categories from the data | |
| const categories: string[] = getUniqueCategories({ selectedYear }); | |
| const years: string[] = ['2023', '2024']; // Add more years as needed | |
| const [mounted, setMounted] = useState(false); | |
| // Setting up state variables using useState hook | |
| const [selectedCategory, setSelectedCategory] = useState<string>('All Categories'); | |
| const [selectedMonth, setSelectedMonth] = useState<string>('All Months'); | |
| const [selectedYear, setSelectedYear] = useState<string>('2024'); | |
| const [windowWidth, setWindowWidth] = useState<number>(0); | |
| const [currentData, setCurrentData] = useState<{ | |
| expensesData: { [key: string]: ExpenseItem[] }; | |
| expensesLinkData: ExpensesLinkItem[]; | |
| }>({ | |
| expensesData: ExpensesData, // Use JSON data as initial value | |
| expensesLinkData: ExpensesLinkData | |
| }); | |
| // Extracting unique categories from the data | |
| const categories: string[] = getUniqueCategories({ selectedYear }); | |
| const years: string[] = useMemo(() => { | |
| try { | |
| // Get all available year folders | |
| const availableYears = Object.keys(require.context( | |
| '../../config/finance/json-data', | |
| true, | |
| /\d{4}\/Expenses\.json$/ | |
| )).map(path => path.match(/(\d{4})/)?.[1] ?? ''); | |
| return [...new Set(availableYears)].sort().reverse(); | |
| } catch { | |
| return ['2023', '2024']; // Fallback to default years | |
| } | |
| }, []); |
@derberg updates the data so i think it's intentional. |
|
@SahilDahekar The deploy preview looks good, and everything is working, but I think the code can be improved. You have created new files and made changes in multiple places, but I don’t think that much work is needed for this. You just need to update the script that handles the JSON data so that it generates files in the same year-wise structure as the YAML files under the json-data folder. You can then use that data for the bar chart. You'll just need to add a dropdown and make some modifications to the BarChartComponent to make it work. I think this should be enough |
|
@vishvamsinh28 Every change i made is necessary :
Even after this the following changes should be made :
|
|
@SahilDahekar The changes you made definitely work and are necessary, but they might not be the best approach. Maybe this can be done with less code without making so many changes.
Try implementing the changes I mentioned in this comment #3658 (comment) |
@vishvamsinh28 Is it okay if I get back to you on Thursday? I have a quiz and one group project due by tomorrow so... |
|
@JeelRajodiya Sure, no problem! I tested it locally, and the script needs to be modified slightly so that we can have the previous year's JSON data. Maybe this change can be made in this PR. Good luck with your quiz and project :) |
|
@SahilDahekar Code coverage has dropped for index.js; it should be at 100%. Please fix that as well |
@vishvamsinh28 Okay the barchart is rendered correctly no doubt about that but as you said displaying categories with 0 amount spent doesn't make sense right . So we can remove the extra categories that are anyways going to show 0 expense.
The json files are all in the same structure as yml files under json-data
The generated json files are used directly to render data to barchart no fancy calculations Also to cut down on the changes i made can you point out some of the changes that seem unnecessary i can remove those . And will work on the code coverage part . |
|
@SahilDahekar I'm not saying there are any unnecessary changes; they are all needed to make it work. But I'm just suggesting that maybe it could be done with less work and code @anshgoyalevil @akshatnema what do you think or maybe we should go with this approach ? |
|
@SahilDahekar Maybe you can revert to this #2038 approach for the index files. It will create json-data in the same structure as the YAML files but under json-data. Instead of importing the files, read them and store the data in a variable like this: { 2023: {}, 2024: {} }. We can get the latest year from this, and it could also be used for charts to display different year-wise data in a dropdown. This approach might be easier and require less effort. I haven’t tested it locally, but it seems doable and should simplify the process. |
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
components/FinancialSummary/Card.tsx (1)
40-42: Improve accessibility and user experience for clickable items.The clickable div should be a button with proper accessibility attributes and visual feedback.
Apply this diff to improve accessibility:
- <div className='m-2 text-sm' onClick={() => handleExpenseClick(item.Category)}> - {item.Category} - </div> + <button + className='m-2 text-sm hover:text-blue-600 focus:outline-none focus:ring-2 focus:ring-blue-500 rounded' + onClick={() => handleExpenseClick(item.Category)} + aria-label={`View details for ${item.Category}`} + > + {item.Category} + </button>
♻️ Duplicate comments (4)
components/FinancialSummary/ExpensesCard.tsx (1)
12-13:⚠️ Potential issueAdd error handling for data loading.
The component should handle potential errors when loading year data and provide a fallback UI.
Apply this diff to add error handling:
export default function ExpensesCard({ year }: { year: string }) { - const { expensesData, expensesLinkData } = loadYearData(year); + const [error, setError] = useState<Error | null>(null); + const [data, setData] = useState<ReturnType<typeof loadYearData> | null>(null); + + useEffect(() => { + try { + const yearData = loadYearData(year); + setData(yearData); + setError(null); + } catch (err) { + setError(err instanceof Error ? err : new Error('Failed to load data')); + setData(null); + } + }, [year]); + + if (error) { + return <div className="text-red-500">Error loading data: {error.message}</div>; + } + + if (!data) { + return <div>Loading...</div>; + } + + const { expensesData, expensesLinkData } = data;components/FinancialSummary/BarChartComponent.tsx (3)
34-35: 🛠️ Refactor suggestionMake years array dynamic.
The years array is hardcoded, which will require manual updates as time progresses.
As suggested in a previous review, consider generating the years array dynamically based on available data.
38-53:⚠️ Potential issueFix useEffect return type and add loading state.
The useEffect hook is causing a pipeline failure and requires proper typing.
As suggested in a previous review, fix the useEffect return type and consider adding a loading state.
55-69: 🛠️ Refactor suggestionAdd error handling to year data loading effect.
The year data loading effect should handle potential errors and provide user feedback.
As suggested in a previous review, add error handling to the year data loading effect.
🧹 Nitpick comments (4)
utils/loadYearData.ts (1)
35-46: Improve error handling with custom error types.Consider creating a custom error type for better error handling and type safety.
Apply this diff to improve error handling:
+class YearDataError extends Error { + constructor(message: string, public year: string) { + super(message); + this.name = 'YearDataError'; + } +} export const loadYearData = (year: string): { expensesData: { [key: string]: ExpenseItem[] }, expensesLinkData: ExpensesLinkItem[] } => { try { const yearData = YEAR_DATA_MAP[year]; if (!yearData) { - throw new Error(`No data available for year ${year}`); + throw new YearDataError(`No data available for year ${year}`, year); } const { expenses: expensesData, links: expensesLinkData } = yearData; return { expensesData, expensesLinkData }; } catch (error) { - console.error(`Failed to load data for year ${year}:`, error); + if (error instanceof YearDataError) { + console.error(`Year data error: ${error.message}`); + } else { + console.error(`Unexpected error loading data for year ${year}:`, error); + } return { expensesData: {}, expensesLinkData: [] }; } };🧰 Tools
🪛 ESLint
[error] 35-35: Replace
year:·string):·{·expensesData:·{·[key:·string]:·ExpenseItem[]·},with⏎··year:·string⏎):·{·expensesData:·{·[key:·string]:·ExpenseItem[]·};(prettier/prettier)
[error] 37-37: Expected blank line after variable declarations.
(newline-after-var)
[error] 38-40: Expected blank line before this statement.
(padding-line-between-statements)
[error] 41-41: Expected blank line after variable declarations.
(newline-after-var)
[error] 42-42: Expected blank line before this statement.
(padding-line-between-statements)
[error] 45-45: Expected blank line before this statement.
(padding-line-between-statements)
tests/index.test.js (1)
82-82: Remove unused variable.The
readFileSyncMockvariable is assigned but never used.Apply this diff to fix the unused variable:
-const readFileSyncMock = jest.spyOn(fs, 'readFileSync').mockImplementation((path) => { +jest.spyOn(fs, 'readFileSync').mockImplementation((path) => {🧰 Tools
🪛 ESLint
[error] 82-82: 'readFileSyncMock' is assigned a value but never used.
(no-unused-vars)
components/FinancialSummary/BarChartComponent.tsx (2)
75-81: Optimize data filtering performance.The current implementation filters data on every render. Consider memoizing the filtered data to improve performance.
Apply this diff to optimize the filtering:
+import { useMemo } from 'react'; -const filteredData: ExpenseItem[] = Object.entries(currentData.expensesData).flatMap(([month, entries]) => +const filteredData: ExpenseItem[] = useMemo(() => + Object.entries(currentData.expensesData).flatMap(([month, entries]) => selectedMonth === 'All Months' || selectedMonth === month ? entries.filter( (entry: ExpenseItem) => selectedCategory === 'All Categories' || entry.Category === selectedCategory ) : [] - ); + ), [currentData.expensesData, selectedMonth, selectedCategory]);
188-188: Improve mobile layout.The ExpensesCard component is conditionally rendered based on window width, which could cause layout shifts during initial load.
Consider using CSS media queries instead of JavaScript for responsive design:
-{windowWidth && windowWidth < 900 ? <ExpensesCard year={selectedYear} /> : null} +<div className="md:hidden"> + <ExpensesCard year={selectedYear} /> +</div>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
components/FinancialSummary/BarChartComponent.tsx(6 hunks)components/FinancialSummary/Card.tsx(1 hunks)components/FinancialSummary/ExpensesCard.tsx(1 hunks)tests/index.test.js(1 hunks)utils/getUniqueCategories.ts(1 hunks)utils/loadYearData.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
tests/index.test.js
[error] 49-49: Replace '2023' with 2023
(prettier/prettier)
[error] 50-50: Replace 'January' with January
(prettier/prettier)
[error] 55-55: Replace '2022' with 2022
(prettier/prettier)
[error] 56-56: Replace 'January' with January
(prettier/prettier)
[error] 64-64: Replace '2023' with 2023
(prettier/prettier)
[error] 68-68: Replace '2022' with 2022
(prettier/prettier)
[error] 81-81: Delete ····
(prettier/prettier)
[error] 82-82: 'readFileSyncMock' is assigned a value but never used.
(no-unused-vars)
[error] 96-96: Replace 'January' with January
(prettier/prettier)
[error] 103-103: Replace call·=>· with (call)·=>
(prettier/prettier)
[error] 106-106: Delete ····
(prettier/prettier)
[error] 116-116: Replace (call·=>· with ((call)·=>
(prettier/prettier)
[error] 131-131: Replace path with (path)
(prettier/prettier)
[error] 139-139: Delete ····
(prettier/prettier)
[error] 141-141: Replace call·=>· with (call)·=>
(prettier/prettier)
[error] 144-144: Delete ····
(prettier/prettier)
utils/getUniqueCategories.ts
[error] 9-9: A space is required after '{'.
(object-curly-spacing)
[error] 9-9: Replace selectedYear,·selectedMonth}·:·{selectedYear:·string;·selectedMonth:·string with ⏎··selectedYear,⏎··selectedMonth⏎}:·{⏎··selectedYear:·string;⏎··selectedMonth:·string;⏎
(prettier/prettier)
[error] 9-9: A space is required before '}'.
(object-curly-spacing)
[error] 10-10: Replace ···· with ··
(prettier/prettier)
[error] 11-11: Delete ··
(prettier/prettier)
[error] 13-13: Replace ···· with ··
(prettier/prettier)
[error] 14-14: Delete ··
(prettier/prettier)
[error] 15-15: Delete ····
(prettier/prettier)
[error] 15-21: Using 'ForInStatement' is not allowed.
(no-restricted-syntax)
[error] 15-21: The body of a for-in should be wrapped in an if statement to filter unwanted properties from the prototype.
(guard-for-in)
[error] 16-16: Replace ············ with ······
(prettier/prettier)
[error] 17-17: Delete ········
(prettier/prettier)
[error] 18-18: Replace ···················· with ··········
(prettier/prettier)
[error] 19-19: Replace ················ with ········
(prettier/prettier)
[error] 20-20: Delete ······
(prettier/prettier)
[error] 21-21: Replace ········ with ····
(prettier/prettier)
[error] 22-22: Delete ··
(prettier/prettier)
[error] 23-23: Replace ········ with ····
(prettier/prettier)
[error] 24-24: Delete ····
(prettier/prettier)
[error] 24-24: Expected blank line after variable declarations.
(newline-after-var)
[error] 25-25: Replace ········ with ····
(prettier/prettier)
[error] 25-29: Expected blank line before this statement.
(padding-line-between-statements)
[error] 26-26: Replace ············ with ······
(prettier/prettier)
[error] 27-27: Delete ········
(prettier/prettier)
[error] 28-28: Replace ············ with ······
(prettier/prettier)
[error] 29-29: Replace ········ with ····
(prettier/prettier)
[error] 30-30: Replace ···· with ··
(prettier/prettier)
[error] 31-31: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 31-31: Delete ····
(prettier/prettier)
utils/loadYearData.ts
[error] 1-7: Run autofix to sort these imports!
(simple-import-sort/imports)
[error] 19-22: Unnecessarily quoted property '2023' found.
(quote-props)
[error] 23-26: Unnecessarily quoted property '2024' found.
(quote-props)
[error] 35-35: Replace year:·string):·{·expensesData:·{·[key:·string]:·ExpenseItem[]·}, with ⏎··year:·string⏎):·{·expensesData:·{·[key:·string]:·ExpenseItem[]·};
(prettier/prettier)
[error] 37-37: Expected blank line after variable declarations.
(newline-after-var)
[error] 38-40: Expected blank line before this statement.
(padding-line-between-statements)
[error] 41-41: Expected blank line after variable declarations.
(newline-after-var)
[error] 42-42: Expected blank line before this statement.
(padding-line-between-statements)
[error] 45-45: Expected blank line before this statement.
(padding-line-between-statements)
[error] 47-47: Newline required at end of file but not found.
(eol-last)
[error] 47-47: Insert ⏎
(prettier/prettier)
🪛 GitHub Actions: PR testing - if Node project
tests/index.test.js
[error] 102-102: TypeError: yearLinks.forEach is not a function
[error] 145-145: TypeError: Cannot read properties of undefined (reading '1')
| for (const month in expensesData) { | ||
| expensesData[month as keyof typeof expensesData].forEach((entry: { Category: string }) => { | ||
| if (!allCategories.includes(entry.Category)) { | ||
| allCategories.push(entry.Category); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Replace for...in loop with Object.entries().
Using for...in loop on objects can be unsafe as it iterates over the prototype chain. Use Object.entries() instead.
Apply this diff to fix the issue:
- for (const month in expensesData) {
- expensesData[month as keyof typeof expensesData].forEach((entry: { Category: string }) => {
- if (!allCategories.includes(entry.Category)) {
- allCategories.push(entry.Category);
- }
- });
- }
+ Object.entries(expensesData).forEach(([_, monthData]) => {
+ monthData.forEach((entry: { Category: string }) => {
+ if (!allCategories.includes(entry.Category)) {
+ allCategories.push(entry.Category);
+ }
+ });
+ });📝 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.
| for (const month in expensesData) { | |
| expensesData[month as keyof typeof expensesData].forEach((entry: { Category: string }) => { | |
| if (!allCategories.includes(entry.Category)) { | |
| allCategories.push(entry.Category); | |
| } | |
| }); | |
| } | |
| Object.entries(expensesData).forEach(([_, monthData]) => { | |
| monthData.forEach((entry: { Category: string }) => { | |
| if (!allCategories.includes(entry.Category)) { | |
| allCategories.push(entry.Category); | |
| } | |
| }); | |
| }); |
🧰 Tools
🪛 ESLint
[error] 15-15: Delete ····
(prettier/prettier)
[error] 15-21: Using 'ForInStatement' is not allowed.
(no-restricted-syntax)
[error] 15-21: The body of a for-in should be wrapped in an if statement to filter unwanted properties from the prototype.
(guard-for-in)
[error] 16-16: Replace ············ with ······
(prettier/prettier)
[error] 17-17: Delete ········
(prettier/prettier)
[error] 18-18: Replace ···················· with ··········
(prettier/prettier)
[error] 19-19: Replace ················ with ········
(prettier/prettier)
[error] 20-20: Delete ······
(prettier/prettier)
[error] 21-21: Replace ········ with ····
(prettier/prettier)
| test('should handle missing expense files gracefully', async () => { | ||
| jest.spyOn(fs, 'readdirSync').mockReturnValue(['2023']); | ||
| jest.spyOn(fs, 'existsSync').mockImplementation(path => !path.includes('Expenses.json')); | ||
| jest.spyOn(fs, 'mkdirSync').mockImplementation(() => {}); | ||
| jest.spyOn(fs, 'writeFileSync').mockImplementation(() => {}); | ||
| jest.spyOn(fs, 'readFileSync').mockReturnValue('[]'); | ||
|
|
||
| await start(); | ||
|
|
||
| const writeFileSyncMock = fs.writeFileSync; | ||
|
|
||
| // Verify empty combined data was written | ||
| const combinedExpensesCall = writeFileSyncMock.mock.calls.find(call => | ||
| call[0].includes('All_years/Expenses.json') | ||
| ); | ||
|
|
||
| expect(JSON.parse(combinedExpensesCall[1])).toEqual({}); | ||
|
|
||
| fs.readdirSync.mockRestore(); | ||
| fs.existsSync.mockRestore(); | ||
| fs.readFileSync.mockRestore(); | ||
| fs.writeFileSync.mockRestore(); | ||
| fs.mkdirSync.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
Fix pipeline failure in missing files test case.
The test case is failing due to a TypeError when accessing undefined property. The test needs to ensure that the mock data structure matches what the implementation expects.
Apply this diff to fix the test:
test('should handle missing expense files gracefully', async () => {
jest.spyOn(fs, 'readdirSync').mockReturnValue(['2023']);
jest.spyOn(fs, 'existsSync').mockImplementation(path => !path.includes('Expenses.json'));
jest.spyOn(fs, 'mkdirSync').mockImplementation(() => {});
- jest.spyOn(fs, 'writeFileSync').mockImplementation(() => {});
+ const writeFileSyncMock = jest.spyOn(fs, 'writeFileSync').mockImplementation(() => {});
jest.spyOn(fs, 'readFileSync').mockReturnValue('[]');
await start();
- const writeFileSyncMock = fs.writeFileSync;
-
// Verify empty combined data was written
const combinedExpensesCall = writeFileSyncMock.mock.calls.find(call =>
call[0].includes('All_years/Expenses.json')
);
expect(JSON.parse(combinedExpensesCall[1])).toEqual({});📝 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.
| test('should handle missing expense files gracefully', async () => { | |
| jest.spyOn(fs, 'readdirSync').mockReturnValue(['2023']); | |
| jest.spyOn(fs, 'existsSync').mockImplementation(path => !path.includes('Expenses.json')); | |
| jest.spyOn(fs, 'mkdirSync').mockImplementation(() => {}); | |
| jest.spyOn(fs, 'writeFileSync').mockImplementation(() => {}); | |
| jest.spyOn(fs, 'readFileSync').mockReturnValue('[]'); | |
| await start(); | |
| const writeFileSyncMock = fs.writeFileSync; | |
| // Verify empty combined data was written | |
| const combinedExpensesCall = writeFileSyncMock.mock.calls.find(call => | |
| call[0].includes('All_years/Expenses.json') | |
| ); | |
| expect(JSON.parse(combinedExpensesCall[1])).toEqual({}); | |
| fs.readdirSync.mockRestore(); | |
| fs.existsSync.mockRestore(); | |
| fs.readFileSync.mockRestore(); | |
| fs.writeFileSync.mockRestore(); | |
| fs.mkdirSync.mockRestore(); | |
| }); | |
| test('should handle missing expense files gracefully', async () => { | |
| jest.spyOn(fs, 'readdirSync').mockReturnValue(['2023']); | |
| jest.spyOn(fs, 'existsSync').mockImplementation(path => !path.includes('Expenses.json')); | |
| jest.spyOn(fs, 'mkdirSync').mockImplementation(() => {}); | |
| const writeFileSyncMock = jest.spyOn(fs, 'writeFileSync').mockImplementation(() => {}); | |
| jest.spyOn(fs, 'readFileSync').mockReturnValue('[]'); | |
| await start(); | |
| // Verify empty combined data was written | |
| const combinedExpensesCall = writeFileSyncMock.mock.calls.find(call => | |
| call[0].includes('All_years/Expenses.json') | |
| ); | |
| expect(JSON.parse(combinedExpensesCall[1])).toEqual({}); | |
| fs.readdirSync.mockRestore(); | |
| fs.existsSync.mockRestore(); | |
| fs.readFileSync.mockRestore(); | |
| fs.writeFileSync.mockRestore(); | |
| fs.mkdirSync.mockRestore(); | |
| }); |
🧰 Tools
🪛 ESLint
[error] 131-131: Replace path with (path)
(prettier/prettier)
[error] 139-139: Delete ····
(prettier/prettier)
[error] 141-141: Replace call·=>· with (call)·=>
(prettier/prettier)
[error] 144-144: Delete ····
(prettier/prettier)
🪛 GitHub Actions: PR testing - if Node project
[error] 145-145: TypeError: Cannot read properties of undefined (reading '1')
| test('should process multiple years of finance data and create combined data', async () => { | ||
| const mockYearData = { | ||
| '2023': { | ||
| 'January': [ | ||
| { Category: 'Hosting', Amount: '100.00' }, | ||
| { Category: 'Marketing', Amount: '200.00' } | ||
| ] | ||
| }, | ||
| '2022': { | ||
| 'January': [ | ||
| { Category: 'Hosting', Amount: '50.00' }, | ||
| { Category: 'Development', Amount: '300.00' } | ||
| ] | ||
| } | ||
| }; | ||
|
|
||
| const mockLinks = { | ||
| '2023': [ | ||
| { category: 'Hosting', link: 'host.com' }, | ||
| { category: 'Marketing', link: 'market.com' } | ||
| ], | ||
| '2022': [ | ||
| { category: 'Development', link: 'dev.com' }, | ||
| { category: 'Hosting', link: 'oldhost.com' } | ||
| ] | ||
| }; | ||
|
|
||
| jest.spyOn(fs, 'readdirSync').mockImplementation((path) => { | ||
| if (path.includes('finance')) return ['2023', '2022', 'json-data']; | ||
| return []; | ||
| }); | ||
|
|
||
| jest.spyOn(fs, 'existsSync').mockReturnValue(true); | ||
| jest.spyOn(fs, 'mkdirSync').mockImplementation(() => {}); | ||
|
|
||
| const readFileSyncMock = jest.spyOn(fs, 'readFileSync').mockImplementation((path) => { | ||
| if (path.includes('2023/Expenses.json')) return JSON.stringify(mockYearData['2023']); | ||
| if (path.includes('2022/Expenses.json')) return JSON.stringify(mockYearData['2022']); | ||
| if (path.includes('2023/ExpensesLink.json')) return JSON.stringify(mockLinks['2023']); | ||
| if (path.includes('2022/ExpensesLink.json')) return JSON.stringify(mockLinks['2022']); | ||
| return '{}'; | ||
| }); | ||
|
|
||
| const writeFileSyncMock = jest.spyOn(fs, 'writeFileSync').mockImplementation(() => {}); | ||
|
|
||
| await start(); | ||
|
|
||
| // Verify combined expenses were calculated correctly | ||
| const expectedCombinedExpenses = { | ||
| 'January': [ | ||
| { Category: 'Hosting', Amount: '150.00' }, | ||
| { Category: 'Marketing', Amount: '200.00' }, | ||
| { Category: 'Development', Amount: '300.00' } | ||
| ] | ||
| }; | ||
|
|
||
| const combinedExpensesCall = writeFileSyncMock.mock.calls.find(call => | ||
| call[0].includes('All_years/Expenses.json') | ||
| ); | ||
|
|
||
| expect(JSON.parse(combinedExpensesCall[1])).toEqual(expectedCombinedExpenses); | ||
|
|
||
| // Verify unique links were combined | ||
| const expectedCombinedLinks = [ | ||
| { category: 'Hosting', link: 'host.com' }, | ||
| { category: 'Marketing', link: 'market.com' }, | ||
| { category: 'Development', link: 'dev.com' } | ||
| ]; | ||
|
|
||
| const combinedLinksCall = writeFileSyncMock.mock.calls.find(call => | ||
| call[0].includes('All_years/ExpensesLink.json') | ||
| ); | ||
|
|
||
| expect(JSON.parse(combinedLinksCall[1])).toEqual(expectedCombinedLinks); | ||
|
|
||
| fs.readdirSync.mockRestore(); | ||
| fs.existsSync.mockRestore(); | ||
| fs.readFileSync.mockRestore(); | ||
| fs.writeFileSync.mockRestore(); | ||
| fs.mkdirSync.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
Fix pipeline failure in multiple years test case.
The test case is failing due to a TypeError indicating that yearLinks.forEach is not a function. This suggests that the test data structure doesn't match what the implementation expects.
Apply this diff to fix the test data structure:
const mockYearData = {
'2023': {
'January': [
{ Category: 'Hosting', Amount: '100.00' },
{ Category: 'Marketing', Amount: '200.00' }
]
},
'2022': {
'January': [
{ Category: 'Hosting', Amount: '50.00' },
{ Category: 'Development', Amount: '300.00' }
]
}
};
-const mockLinks = {
- '2023': [
+const mockLinks = {
+ '2023': {
+ links: [
{ category: 'Hosting', link: 'host.com' },
{ category: 'Marketing', link: 'market.com' }
- ],
- '2022': [
+ ]
+ },
+ '2022': {
+ links: [
{ category: 'Development', link: 'dev.com' },
{ category: 'Hosting', link: 'oldhost.com' }
- ]
+ ]
+ }
};📝 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.
| test('should process multiple years of finance data and create combined data', async () => { | |
| const mockYearData = { | |
| '2023': { | |
| 'January': [ | |
| { Category: 'Hosting', Amount: '100.00' }, | |
| { Category: 'Marketing', Amount: '200.00' } | |
| ] | |
| }, | |
| '2022': { | |
| 'January': [ | |
| { Category: 'Hosting', Amount: '50.00' }, | |
| { Category: 'Development', Amount: '300.00' } | |
| ] | |
| } | |
| }; | |
| const mockLinks = { | |
| '2023': [ | |
| { category: 'Hosting', link: 'host.com' }, | |
| { category: 'Marketing', link: 'market.com' } | |
| ], | |
| '2022': [ | |
| { category: 'Development', link: 'dev.com' }, | |
| { category: 'Hosting', link: 'oldhost.com' } | |
| ] | |
| }; | |
| jest.spyOn(fs, 'readdirSync').mockImplementation((path) => { | |
| if (path.includes('finance')) return ['2023', '2022', 'json-data']; | |
| return []; | |
| }); | |
| jest.spyOn(fs, 'existsSync').mockReturnValue(true); | |
| jest.spyOn(fs, 'mkdirSync').mockImplementation(() => {}); | |
| const readFileSyncMock = jest.spyOn(fs, 'readFileSync').mockImplementation((path) => { | |
| if (path.includes('2023/Expenses.json')) return JSON.stringify(mockYearData['2023']); | |
| if (path.includes('2022/Expenses.json')) return JSON.stringify(mockYearData['2022']); | |
| if (path.includes('2023/ExpensesLink.json')) return JSON.stringify(mockLinks['2023']); | |
| if (path.includes('2022/ExpensesLink.json')) return JSON.stringify(mockLinks['2022']); | |
| return '{}'; | |
| }); | |
| const writeFileSyncMock = jest.spyOn(fs, 'writeFileSync').mockImplementation(() => {}); | |
| await start(); | |
| // Verify combined expenses were calculated correctly | |
| const expectedCombinedExpenses = { | |
| 'January': [ | |
| { Category: 'Hosting', Amount: '150.00' }, | |
| { Category: 'Marketing', Amount: '200.00' }, | |
| { Category: 'Development', Amount: '300.00' } | |
| ] | |
| }; | |
| const combinedExpensesCall = writeFileSyncMock.mock.calls.find(call => | |
| call[0].includes('All_years/Expenses.json') | |
| ); | |
| expect(JSON.parse(combinedExpensesCall[1])).toEqual(expectedCombinedExpenses); | |
| // Verify unique links were combined | |
| const expectedCombinedLinks = [ | |
| { category: 'Hosting', link: 'host.com' }, | |
| { category: 'Marketing', link: 'market.com' }, | |
| { category: 'Development', link: 'dev.com' } | |
| ]; | |
| const combinedLinksCall = writeFileSyncMock.mock.calls.find(call => | |
| call[0].includes('All_years/ExpensesLink.json') | |
| ); | |
| expect(JSON.parse(combinedLinksCall[1])).toEqual(expectedCombinedLinks); | |
| fs.readdirSync.mockRestore(); | |
| fs.existsSync.mockRestore(); | |
| fs.readFileSync.mockRestore(); | |
| fs.writeFileSync.mockRestore(); | |
| fs.mkdirSync.mockRestore(); | |
| }); | |
| test('should process multiple years of finance data and create combined data', async () => { | |
| const mockYearData = { | |
| '2023': { | |
| 'January': [ | |
| { Category: 'Hosting', Amount: '100.00' }, | |
| { Category: 'Marketing', Amount: '200.00' } | |
| ] | |
| }, | |
| '2022': { | |
| 'January': [ | |
| { Category: 'Hosting', Amount: '50.00' }, | |
| { Category: 'Development', Amount: '300.00' } | |
| ] | |
| } | |
| }; | |
| const mockLinks = { | |
| '2023': { | |
| links: [ | |
| { category: 'Hosting', link: 'host.com' }, | |
| { category: 'Marketing', link: 'market.com' } | |
| ] | |
| }, | |
| '2022': { | |
| links: [ | |
| { category: 'Development', link: 'dev.com' }, | |
| { category: 'Hosting', link: 'oldhost.com' } | |
| ] | |
| } | |
| }; | |
| jest.spyOn(fs, 'readdirSync').mockImplementation((path) => { | |
| if (path.includes('finance')) return ['2023', '2022', 'json-data']; | |
| return []; | |
| }); | |
| jest.spyOn(fs, 'existsSync').mockReturnValue(true); | |
| jest.spyOn(fs, 'mkdirSync').mockImplementation(() => {}); | |
| const readFileSyncMock = jest.spyOn(fs, 'readFileSync').mockImplementation((path) => { | |
| if (path.includes('2023/Expenses.json')) return JSON.stringify(mockYearData['2023']); | |
| if (path.includes('2022/Expenses.json')) return JSON.stringify(mockYearData['2022']); | |
| if (path.includes('2023/ExpensesLink.json')) return JSON.stringify(mockLinks['2023']); | |
| if (path.includes('2022/ExpensesLink.json')) return JSON.stringify(mockLinks['2022']); | |
| return '{}'; | |
| }); | |
| const writeFileSyncMock = jest.spyOn(fs, 'writeFileSync').mockImplementation(() => {}); | |
| await start(); | |
| // Verify combined expenses were calculated correctly | |
| const expectedCombinedExpenses = { | |
| 'January': [ | |
| { Category: 'Hosting', Amount: '150.00' }, | |
| { Category: 'Marketing', Amount: '200.00' }, | |
| { Category: 'Development', Amount: '300.00' } | |
| ] | |
| }; | |
| const combinedExpensesCall = writeFileSyncMock.mock.calls.find(call => | |
| call[0].includes('All_years/Expenses.json') | |
| ); | |
| expect(JSON.parse(combinedExpensesCall[1])).toEqual(expectedCombinedExpenses); | |
| // Verify unique links were combined | |
| const expectedCombinedLinks = [ | |
| { category: 'Hosting', link: 'host.com' }, | |
| { category: 'Marketing', link: 'market.com' }, | |
| { category: 'Development', link: 'dev.com' } | |
| ]; | |
| const combinedLinksCall = writeFileSyncMock.mock.calls.find(call => | |
| call[0].includes('All_years/ExpensesLink.json') | |
| ); | |
| expect(JSON.parse(combinedLinksCall[1])).toEqual(expectedCombinedLinks); | |
| fs.readdirSync.mockRestore(); | |
| fs.existsSync.mockRestore(); | |
| fs.readFileSync.mockRestore(); | |
| fs.writeFileSync.mockRestore(); | |
| fs.mkdirSync.mockRestore(); | |
| }); |
🧰 Tools
🪛 ESLint
[error] 49-49: Replace '2023' with 2023
(prettier/prettier)
[error] 50-50: Replace 'January' with January
(prettier/prettier)
[error] 55-55: Replace '2022' with 2022
(prettier/prettier)
[error] 56-56: Replace 'January' with January
(prettier/prettier)
[error] 64-64: Replace '2023' with 2023
(prettier/prettier)
[error] 68-68: Replace '2022' with 2022
(prettier/prettier)
[error] 81-81: Delete ····
(prettier/prettier)
[error] 82-82: 'readFileSyncMock' is assigned a value but never used.
(no-unused-vars)
[error] 96-96: Replace 'January' with January
(prettier/prettier)
[error] 103-103: Replace call·=>· with (call)·=>
(prettier/prettier)
[error] 106-106: Delete ····
(prettier/prettier)
[error] 116-116: Replace (call·=>· with ((call)·=>
(prettier/prettier)
🪛 GitHub Actions: PR testing - if Node project
[error] 102-102: TypeError: yearLinks.forEach is not a function
|
@vishvamsinh28 Actually I updated the script which converts the yaml data into json for all the years. If you'd like, can I share that with you? |
|
@AlexiusTatius yes 👍 |
|
@vishvamsinh28 Here is the new function which I added in async function buildFinanceInfoListAllYears({currentDir, configDir, financeDir, yearsList, jsonDataAllYearsDirName}){
// Ensure the directory exists before writing the files
const jsonDataAllYearsDir = resolve(currentDir, configDir, financeDir, jsonDataAllYearsDirName);
await mkdir(jsonDataAllYearsDir, { recursive: true });
const basePath = resolve(currentDir, configDir, financeDir);
const allYearsExpenses = {};
const allYearsExpensesLink = [];
let uniqueExpensesLink = [];
for (const year of yearsList) {
const currentYearExpensesPath = resolve(basePath, year, 'Expenses.yml');
const currentYearExpenses = await readJsonReturnObject(currentYearExpensesPath);
allYearsExpenses[year] = currentYearExpenses;
const currentYearExpensesLinkPath = resolve(basePath, year, 'ExpensesLink.yml');
const currentYearExpensesLink = await readJsonReturnObject(currentYearExpensesLinkPath);
allYearsExpensesLink.push(...currentYearExpensesLink);
// Remove duplicates from allYearsExpensesLink
uniqueExpensesLink = Array.from(
new Set(allYearsExpensesLink.map(item => `${item.category}|${item.link}`))
)
.map(compositeKey => {
const [category, link] = compositeKey.split('|');
return { category, link };
})
}
const expensesJsonPath = resolve(jsonDataAllYearsDir, 'Expenses.json');
await writeFile(expensesJsonPath, JSON.stringify(allYearsExpenses));
const expensesLinkJsonPath = resolve(jsonDataAllYearsDir, 'ExpensesLink.json');
await writeFile(expensesLinkJsonPath, JSON.stringify(uniqueExpensesLink));
}another function which I created:
const { promises: { readFile } } = require('fs');
const { convertToJson: convertToJSObject } = require("../utils");
module.exports = async function readJsonReturnObject(readPath){
let readContent;
// Attempt to read the file
try {
readContent = await readFile(readPath, 'utf-8');
} catch (err) {
throw new Error(`Error while reading file\nError: ${err}`);
}
// Attempt to convert content to JSON
try {
jsObject = convertToJSObject(readContent);
} catch (err) {
throw new Error(`Error while conversion\nError: ${err}`);
}
return jsObject;
}Here is the output which I receive, {
"2023": {
"January": [
{ "Category": "Ambassador Program", "Amount": "68.95" },
{ "Category": "Google Season of Docs 2022", "Amount": "35.62" },
{ "Category": "Google Season of Docs 2022", "Amount": "1666.67" },
{ "Category": "AsyncAPI Mentorship 2022", "Amount": "1500" },
{ "Category": "AsyncAPI Mentorship 2022", "Amount": "1500" },
{ "Category": "AsyncAPI Mentorship 2022", "Amount": "1500" }
],
"February": [
{ "Category": "Community Manager", "Amount": "1000.39" },
{ "Category": "AsyncAPI Mentorship 2022", "Amount": "1500" }
],
"March": [
{ "Category": "Community Manager", "Amount": "2000.39" },
{ "Category": "AsyncAPI Mentorship 2022", "Amount": "1500" },
{ "Category": "AsyncAPI Mentorship 2022", "Amount": "1500" }
],
"April": [{ "Category": "Community Manager", "Amount": "2000.39" }],
"May": [
{ "Category": "Community Manager", "Amount": "2000.39" },
{ "Category": "Swag Store", "Amount": "75.11" },
{ "Category": "Bounty Program", "Amount": "400" }
],
"June": [
{ "Category": "Community Manager", "Amount": "2000.39" },
{ "Category": "Bounty Program", "Amount": "200" },
{ "Category": "3rd Party Services", "Amount": "28.31" },
{ "Category": "Bounty Program", "Amount": "200" },
{ "Category": "Bounty Program", "Amount": "200" },
{ "Category": "Bounty Program", "Amount": "200" }
],
"July": [
{ "Category": "Community Manager", "Amount": "2000.39" },
{ "Category": "Bounty Program", "Amount": "400" }
],
"August": [
{ "Category": "3rd Party Services", "Amount": "1093.92" },
{ "Category": "Swag Store", "Amount": "15672.02" },
{ "Category": "Bounty Program", "Amount": "800.78" },
{ "Category": "Community Manager", "Amount": "2000.39" }
],
"September": [
{ "Category": "Ambassador Program", "Amount": "139.10" },
{ "Category": "Community Manager", "Amount": "2000.39" }
],
"October": [
{ "Category": "Mentorship Program 2023", "Amount": "5277.78" },
{ "Category": "Community Manager", "Amount": "2000.39" },
{ "Category": "AsyncAPI Conf on Tour 2023", "Amount": "943.07" },
{ "Category": "Swag Store", "Amount": "7893.72" },
{ "Category": "3rd Party Services", "Amount": "247.04" }
],
"November": [
{ "Category": "Mentorship Program 2023", "Amount": "1363.50" },
{ "Category": "Community Manager", "Amount": "2000.39" },
{ "Category": "Swag Store", "Amount": "873.87" }
],
"December": [
{ "Category": "Mentorship Program 2023", "Amount": "675.39" },
{ "Category": "Community Manager", "Amount": "2000.39" },
{ "Category": "AsyncAPI Conf on Tour 2023", "Amount": "1356.34" },
{ "Category": "Swag Store", "Amount": "1415.90" },
{ "Category": "Bounty Program", "Amount": "813.10" }
]
},
"2024": {
"January": [
{ "Category": "JSON Schema Sponsorship", "Amount": "250.00" },
{ "Category": "Swag Store", "Amount": "678.26" },
{ "Category": "Bounty Program", "Amount": "1800.00" },
{ "Category": "Community Manager", "Amount": "2000.00" },
{ "Category": "AsyncAPI Conf on Tour 2023", "Amount": "318.98" }
],
"February": [
{ "Category": "Bounty Program", "Amount": "400.00" },
{ "Category": "JSON Schema Sponsorship", "Amount": "250.00" },
{ "Category": "Community Manager", "Amount": "2000.00" },
{ "Category": "Swag Store", "Amount": "474.35" },
{ "Category": "AsyncAPI Conf on Tour 2023", "Amount": "675.56" },
{ "Category": "Mentorship Program 2023", "Amount": "1650.00" }
],
"March": [
{ "Category": "JSON Schema Sponsorship", "Amount": "250.00" },
{ "Category": "Community Manager", "Amount": "2000.00" },
{ "Category": "Mentorship Program 2023", "Amount": "4950.00" },
{ "Category": "Swag Store", "Amount": "955.03" },
{ "Category": "Bounty Program", "Amount": "200.00" }
],
"April": [
{ "Category": "JSON Schema Sponsorship", "Amount": "250.00" },
{ "Category": "Community Manager", "Amount": "2000.00" },
{ "Category": "Mentorship Program 2023", "Amount": "825.00" },
{ "Category": "Swag Store", "Amount": "607.97" },
{ "Category": "Bounty Program", "Amount": "800.00" }
],
"May": [
{ "Category": "JSON Schema Sponsorship", "Amount": "250.00" },
{ "Category": "Community Manager", "Amount": "2000.00" },
{ "Category": "Mentorship Program 2023", "Amount": "825.00" },
{ "Category": "Swag Store", "Amount": "526.51" },
{ "Category": "Bounty Program", "Amount": "800.00" },
{ "Category": "Community Marketing Specialist", "Amount": "1000.00" }
],
"June": [
{ "Category": "JSON Schema Sponsorship", "Amount": "250.00" },
{ "Category": "Community Manager", "Amount": "2000.00" },
{ "Category": "Community Marketing Specialist", "Amount": "2000.00" },
{ "Category": "Bounty Program", "Amount": "800.00" },
{ "Category": "Swag Store", "Amount": "526.51" }
],
"July": [
{ "Category": "JSON Schema Sponsorship", "Amount": "250.00" },
{ "Category": "Community Manager", "Amount": "2000.00" },
{ "Category": "Bounty Program", "Amount": "1000.00" },
{ "Category": "Swag Store", "Amount": "526.51" },
{ "Category": "Community Marketing Specialist", "Amount": "2000.00" },
{ "Category": "AsyncAPI Conf on Tour 2024", "Amount": "2083.41" }
],
"August": [
{ "Category": "JSON Schema Sponsorship", "Amount": "250.00" },
{ "Category": "Community Manager", "Amount": "2000.00" },
{ "Category": "Bounty Program", "Amount": "1800.00" },
{ "Category": "Swag Store", "Amount": "2556.42" },
{ "Category": "Community Marketing Specialist", "Amount": "2000.00" },
{ "Category": "3rd Party Services", "Amount": "1354.35" },
{ "Category": "AsyncAPI Conf on Tour 2024", "Amount": "1384.70" }
],
"September": [
{ "Category": "Bounty Program", "Amount": "3000.00" },
{ "Category": "Swag Store", "Amount": "736.59" },
{ "Category": "Community Manager", "Amount": "2000.00" },
{ "Category": "Community Marketing Specialist", "Amount": "2000.00" },
{ "Category": "JSON Schema Sponsorship", "Amount": "250.00" }
],
"October": [
{ "Category": "Bounty Program", "Amount": "1000.00" },
{ "Category": "Swag Store", "Amount": "882.12" },
{ "Category": "AsyncAPI Conf on Tour 2024", "Amount": "962.01" },
{ "Category": "Community Manager", "Amount": "2000.00" },
{ "Category": "Community Marketing Specialist", "Amount": "2000.00" },
{ "Category": "JSON Schema Sponsorship", "Amount": "250.00" }
]
}
}
[
{
"category": "Ambassador Program",
"link": "https://github.com/orgs/asyncapi/discussions/425"
},
{
"category": "Google Season of Docs 2023",
"link": "https://github.com/orgs/asyncapi/discussions/961"
},
{
"category": "Swag Store",
"link": "https://github.com/orgs/asyncapi/discussions/710"
},
{
"category": "Bounty Program",
"link": "https://github.com/orgs/asyncapi/discussions/541"
},
{
"category": "3rd Party Services",
"link": "https://github.com/orgs/asyncapi/discussions/295"
},
{
"category": "Community Manager",
"link": "https://github.com/orgs/asyncapi/discussions/515"
},
{
"category": "AsyncAPI Conf on Tour 2023",
"link": "https://github.com/orgs/asyncapi/discussions/598"
},
{
"category": "AsyncAPI Conf on Tour 2024",
"link": "https://github.com/orgs/asyncapi/discussions/1018"
},
{
"category": "Mentorship Program 2023",
"link": "https://github.com/orgs/asyncapi/discussions/689"
},
{
"category": "JSON Schema Sponsorship",
"link": "https://github.com/orgs/asyncapi/discussions/1017"
},
{
"category": "Community Marketing Specialist",
"link": "https://github.com/orgs/asyncapi/discussions/1176"
},
{
"category": "Google Season of Docs 2022",
"link": "https://github.com/orgs/asyncapi/discussions/303"
},
{
"category": "AsyncAPI Mentorship 2022",
"link": "https://github.com/orgs/asyncapi/discussions/284"
}
] |
|
@vishvamsinh28 If needed I can write this in typescript. |
Hii @vishvamsinh28, Do you still require me to lookup into the scripts? I suggest to take a reference from this comment to understand how auto year detection works. I see that in this PR we are creating multiple folders for each year in the Edit: The quiz went well btw. Thank you. |
|
Hi @JeelRajodiya no, it's not needed now. Yes, that's the issue, which is why I suggested this approach #3658 (comment) . We can do auto year detection with it and will also fetch the previous year's data. What do you think about it? Great to hear that your quiz went well! |
|
@vishvamsinh28 I read your suggestion, If I understand correctly you are suggesting to read the json files directly (using something like "fs") and then store them in a variable? I think it is difficult to read the text files because javascript does not allow the client to read files directly from the codebase itself (due to security reasons) |
|
@JeelRajodiya what about creating a single json file? |
|
@AlexiusTatius, that sounds doable to me. I think @vishvamsinh28 might be suggesting the same in his comment, apologies If I misinterpreted your comment @vishvamsinh28. |
|
@AlexiusTatius Yes, we can do that as well @JeelRajodiya For reading JSON data and storing it in a variable, we don't need and have fs on the client side, but we do have fetch. which will work 
|
|
@vishvamsinh28 @JeelRajodiya @AlexiusTatius 2 questions from what i understand ( correct me if am wrong )
const currentYear = new Date().getFullYear();
const previousYear = currentYear - 1;
// Fetch current year data. If not available get previous year data as latest.Let me know your thoughts on this ? |
|
Right. This approach sounds good to me. 🚀 |
Description
Expenses.ymlandExpensesLink.ymlfile for each year.loadYearDatautility function to load data files according to the year selected./financepage.Screenshots
Hydration error which are now resolved
Related issue(s)
Resolves #3653
Summary by CodeRabbit
New Features
Bug Fixes