Initialize vue project + Reports Overview component#1053
Initialize vue project + Reports Overview component#1053samuelsa-adyen wants to merge 2 commits intodevelopfrom
Conversation
samuelsa-adyen
force-pushed
the
feature/initialze-vue-project
branch
from
eeee90a to
5560935
Compare
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes the foundational elements for a new Vue.js application, integrating essential development tools and core functionalities. It provides a robust starting point for future feature development by setting up the project structure, component showcasing with Storybook, internationalization resources, and clear API interaction patterns. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several new files and modifies existing ones to integrate Storybook, internationalization, and BentoPlugin into a Vue.js project. The base URL for API endpoints is hardcoded and should be configured using environment variables. Several dependencies in package.json are outdated, including vite, @storybook/vue3, @storybook/vue3-vite, storybook, cross-env, and vitest, and should be updated for performance improvements and new features. Type safety can be improved in .storybook/main.ts by defining more specific types for plugins and output properties. The logic for VITE_LOCAL_ASSETS in .storybook/main.ts may be inverted and should be clarified. The version ranges for vue-i18n and vue peer dependencies in package.json are wide and could potentially pull in breaking changes, so more restrictive ranges should be considered. The value 10_000 for experimentalMinChunkSize is a magic number and should be replaced with a named constant.
| @@ -0,0 +1,54 @@ | |||
| const baseUrl = 'https://platform-components-external-test.adyen.com/platform-components-external/api/v([0-9]+)'; | |||
There was a problem hiding this comment.
The baseUrl is hardcoded to https://platform-components-external-test.adyen.com. This makes the application less flexible for different environments (e.g., development, staging, production). It's recommended to configure this base URL using environment variables to allow for easy switching between environments without code changes.
| "storybook": "^10.1.10", | ||
| "terser": "^5.46.0", | ||
| "typescript": "^5.8.3", | ||
| "vite": "^7.3.0", |
There was a problem hiding this comment.
The vite dependency is outdated (^7.3.0). The latest stable version is 5.x. Updating Vite is highly recommended for performance improvements, new features, and better compatibility with modern web development practices. This might require reviewing the Vite configuration for any breaking changes.
| 'process.env.VITE_LOCAL_ASSETS': JSON.stringify(process.env.USE_CDN === 'true' ? null : true), | ||
| 'process.env.VITE_VERSION': JSON.stringify('1.0.0'), | ||
| 'process.env.SESSION_ACCOUNT_HOLDER': JSON.stringify(api.session.accountHolder || null), | ||
| 'process.env.SESSION_AUTO_REFRESH': JSON.stringify(api.session.autoRefresh === 'true' || null), |
There was a problem hiding this comment.
Similar to the VITE_LOCAL_ASSETS comment, api.session.autoRefresh === 'true' || null results in null if autoRefresh is not the string 'true'. If the intention is to represent a boolean false or undefined when it's not 'true', using null might be inconsistent with boolean expectations. Consider JSON.stringify(api.session.autoRefresh === 'true' ? true : false) or JSON.stringify(api.session.autoRefresh === 'true' || undefined) for clearer boolean representation.
| const removeInspect = (plugins: any[]): any[] => | ||
| plugins.flat(Infinity).filter((p: any) => !(p && typeof p === 'object' && p.name === 'vite-plugin-inspect')); |
There was a problem hiding this comment.
Using any[] for plugin types bypasses TypeScript's type checking, which can lead to unexpected runtime errors. Consider defining a more specific type for plugins and the individual p elements to leverage TypeScript's benefits fully.
| ...config.build?.rollupOptions, | ||
| output: { | ||
| ...(config.build?.rollupOptions?.output as object), | ||
| experimentalMinChunkSize: 10_000, |
There was a problem hiding this comment.
Using as object for config.build?.rollupOptions?.output is a generic type assertion that weakens type safety. It would be better to define a more specific interface or type for the output property to ensure type correctness.
| "vue-i18n": "^11" | ||
| }, | ||
| "peerDependencies": { | ||
| "vue": "^3.3.0" |
There was a problem hiding this comment.
The vue peer dependency is specified with a wide version range (^3.3.0). While this allows for minor updates, it could potentially pull in versions with breaking changes if a new major version (e.g., 4.0.0) is released that still satisfies ^3.3.0 but introduces breaking changes. Consider pinning to a specific major version or using a more restrictive range if stability is critical.
| "devDependencies": { | ||
| "@storybook/vue3": "^10.1.10", |
There was a problem hiding this comment.
The Storybook dependencies (@storybook/vue3, @storybook/vue3-vite, storybook) are using version ^10.1.10. Storybook has since released major version 8. It's advisable to update these dependencies to the latest stable version (Storybook 8.x) to benefit from new features, performance improvements, and bug fixes. This might involve some migration effort.
| "@storybook/vue3-vite": "^10.1.10", | ||
| "@types/node": "^24.10.13", | ||
| "@vitejs/plugin-vue": "^6.0.3", | ||
| "cross-env": "^10.1.0", |
There was a problem hiding this comment.
The cross-env dependency is outdated (^10.1.0). The latest stable version is 7.0.3. Updating this dependency is recommended to ensure compatibility and benefit from any improvements or bug fixes.
| config.resolve = { | ||
| ...config.resolve, | ||
| alias: { | ||
| ...(config.resolve?.alias as Record<string, string>), |
There was a problem hiding this comment.
Type assertions like as Record<string, string> can be risky if the underlying type is not what's asserted. It's safer to ensure the type is correct through proper typing or handle potential undefined cases explicitly.
| "typescript": "^5.8.3", | ||
| "vite": "^7.3.0", | ||
| "vite-plugin-vue-devtools": "^8.0.5", | ||
| "vitest": "^4.0.18", |
There was a problem hiding this comment.
The vitest dependency is outdated (^4.0.18). The latest stable version is 1.x. Updating Vitest will provide access to the latest testing features, performance enhancements, and bug fixes. This might require reviewing test configurations for any breaking changes.
There was a problem hiding this comment.
Code Review
This pull request initializes a new Vue project with Storybook, Vite, and i18n configurations. It also adds a significant number of data files for country translations. The setup is comprehensive, but I've identified a few areas for improvement concerning dependency management, data quality, and code simplification. There's also a potential bug in how an environment variable is processed in the Storybook configuration that should be addressed.
| 'process.env.SESSION_MAX_AGE_MS': JSON.stringify(api.session.maxAgeMs || null), | ||
| 'process.env.SESSION_PERMISSIONS': JSON.stringify(api.session.permissions || null), | ||
| 'process.env.USE_CDN': JSON.stringify(app.useCdn ?? null), | ||
| 'process.env.VITE_TEST_CDN_ASSETS': JSON.stringify(app.useTestCdn ? true : null), |
There was a problem hiding this comment.
The current check for app.useTestCdn is a truthiness check. Since environment variables are read as strings, a value of 'false' will be treated as true, which is likely not the intended behavior. This could lead to test CDN assets being used incorrectly. To fix this, you should explicitly check if the value is the string 'true'.
| 'process.env.VITE_TEST_CDN_ASSETS': JSON.stringify(app.useTestCdn ? true : null), | |
| 'process.env.VITE_TEST_CDN_ASSETS': JSON.stringify(app.useTestCdn === 'true' ? true : null), |
| export const endpoints = () => | ||
| ({ | ||
| balanceAccounts: `${baseUrl}/balanceAccounts`, | ||
| balances: `${baseUrl}/balanceAccounts/:id/balances`, | ||
| payouts: `${baseUrl}/payouts`, | ||
| payout: `${baseUrl}/payouts/breakdown`, | ||
| transactions: `${baseUrl}/transactions`, | ||
| transaction: `${baseUrl}/transactions/:id`, | ||
| initiateRefund: `${baseUrl}/transactions/:id/refund`, | ||
| transactionsTotals: `${baseUrl}/transactions/totals`, | ||
| downloadTransactions: `${baseUrl}/transactions/download`, | ||
| sessions: '/api/authe/api/v1/sessions', | ||
| setup: `${baseUrl}/setup`, | ||
| sendEngageEvent: `${baseUrl}/uxdsclient/engage`, | ||
| sendTrackEvent: `${baseUrl}/uxdsclient/track`, | ||
| reports: `${baseUrl}/reports`, | ||
| downloadReport: `${baseUrl}/reports/download`, | ||
| stores: `${baseUrl}/stores`, | ||
| capital: { | ||
| anaCredit: `${baseUrl}/capital/grants/missingActions/anaCredit`, | ||
| createOffer: `${baseUrl}/capital/grantOffers/create`, | ||
| dynamicOfferConfig: `${baseUrl}/capital/grantOffers/dynamic/configuration`, | ||
| dynamicOffer: `${baseUrl}/capital/grantOffers/dynamic`, | ||
| grants: `${baseUrl}/capital/grants`, | ||
| requestFunds: `${baseUrl}/capital/grants/:id`, | ||
| signToS: `${baseUrl}/capital/grants/missingActions/signToS`, | ||
| }, | ||
| disputes: { | ||
| accept: `${baseUrl}/disputes/:id/accept`, | ||
| defend: `${baseUrl}/disputes/:id/defend`, | ||
| details: `${baseUrl}/disputes/:id`, | ||
| documents: `${baseUrl}/disputes/:id/documents`, | ||
| download: `${baseUrl}/disputes/:id/documents/download`, | ||
| list: `${baseUrl}/disputes`, | ||
| }, | ||
| payByLink: { | ||
| configuration: `${baseUrl}/paybylink/paymentLinks/:storeId/configuration`, | ||
| countries: `${baseUrl}/paybylink/countries`, | ||
| currencies: `${baseUrl}/paybylink/currencies`, | ||
| details: `${baseUrl}/paybylink/paymentLinks/:id`, | ||
| expire: `${baseUrl}/paybylink/paymentLinks/:id/expire`, | ||
| filters: `${baseUrl}/paybylink/filters`, | ||
| installments: `${baseUrl}/paybylink/installments`, | ||
| list: `${baseUrl}/paybylink/paymentLinks`, | ||
| settings: `${baseUrl}/paybylink/settings/:storeId`, | ||
| themes: `${baseUrl}/paybylink/themes/:id`, | ||
| }, | ||
| datasets: { | ||
| countries: `${datasetBaseUrl}/countries/:locale.json?import`, | ||
| }, | ||
| }) as const; |
There was a problem hiding this comment.
The endpoints export is a function that immediately returns a static object. The function wrapper is unnecessary and can be removed for simplicity and to avoid a minor function call overhead. You can export the object directly as a constant.
export const endpoints = {
balanceAccounts: `${baseUrl}/balanceAccounts`,
balances: `${baseUrl}/balanceAccounts/:id/balances`,
payouts: `${baseUrl}/payouts`,
payout: `${baseUrl}/payouts/breakdown`,
transactions: `${baseUrl}/transactions`,
transaction: `${baseUrl}/transactions/:id`,
initiateRefund: `${baseUrl}/transactions/:id/refund`,
transactionsTotals: `${baseUrl}/transactions/totals`,
downloadTransactions: `${baseUrl}/transactions/download`,
sessions: '/api/authe/api/v1/sessions',
setup: `${baseUrl}/setup`,
sendEngageEvent: `${baseUrl}/uxdsclient/engage`,
sendTrackEvent: `${baseUrl}/uxdsclient/track`,
reports: `${baseUrl}/reports`,
downloadReport: `${baseUrl}/reports/download`,
stores: `${baseUrl}/stores`,
capital: {
anaCredit: `${baseUrl}/capital/grants/missingActions/anaCredit`,
createOffer: `${baseUrl}/capital/grantOffers/create`,
dynamicOfferConfig: `${baseUrl}/capital/grantOffers/dynamic/configuration`,
dynamicOffer: `${baseUrl}/capital/grantOffers/dynamic`,
grants: `${baseUrl}/capital/grants`,
requestFunds: `${baseUrl}/capital/grants/:id`,
signToS: `${baseUrl}/capital/grants/missingActions/signToS`,
},
disputes: {
accept: `${baseUrl}/disputes/:id/accept`,
defend: `${baseUrl}/disputes/:id/defend`,
details: `${baseUrl}/disputes/:id`,
documents: `${baseUrl}/disputes/:id/documents`,
download: `${baseUrl}/disputes/:id/documents/download`,
list: `${baseUrl}/disputes`,
},
payByLink: {
configuration: `${baseUrl}/paybylink/paymentLinks/:storeId/configuration`,
countries: `${baseUrl}/paybylink/countries`,
currencies: `${baseUrl}/paybylink/currencies`,
details: `${baseUrl}/paybylink/paymentLinks/:id`,
expire: `${baseUrl}/paybylink/paymentLinks/:id/expire`,
filters: `${baseUrl}/paybylink/filters`,
installments: `${baseUrl}/paybylink/installments`,
list: `${baseUrl}/paybylink/paymentLinks`,
settings: `${baseUrl}/paybylink/settings/:storeId`,
themes: `${baseUrl}/paybylink/themes/:id`,
},
datasets: {
countries: `${datasetBaseUrl}/countries/:locale.json?import`,
},
} as const;| "devDependencies": { | ||
| "@storybook/vue3": "^10.1.10", | ||
| "@storybook/vue3-vite": "^10.1.10", | ||
| "@types/node": "^24.10.13", |
There was a problem hiding this comment.
The @types/node dependency is pinned to version ^24.10.13, which corresponds to an unstable/nightly release of Node.js. To ensure stability and prevent potential compatibility issues for developers using LTS versions of Node, it's recommended to use types for a stable LTS release, such as Node 20.
| "@types/node": "^24.10.13", | |
| "@types/node": "^20.0.0", |
| { | ||
| "id": "AF", | ||
| "name": "Afghanistan" | ||
| }, |
There was a problem hiding this comment.
The country code AN for 'Netherlands Antilles' is obsolete. This country was dissolved in 2010, and its territories now have new ISO 3166-1 alpha-2 codes: BQ (Bonaire, Sint Eustatius and Saba), CW (Curaçao), and SX (Sint Maarten). Using an obsolete code can lead to data inconsistencies and issues with other systems. Please consider updating to the current codes or removing this entry if it's not required for legacy support.
1 participant
Summary
Tested scenarios
Fixed issue: