Closed sebpalluel closed 6 months ago
The latest updates on your projects. Learn more about Vercel for Git ↗︎
Name | Status | Preview | Updated (UTC) |
---|---|---|---|
back-office | ✅ Ready (Inspect) | Visit Preview | May 2, 2024 8:00am |
marketplace | ✅ Ready (Inspect) | Visit Preview | May 2, 2024 8:00am |
unlock | ✅ Ready (Inspect) | Visit Preview | May 2, 2024 8:00am |
PR Description updated to latest commit (https://github.com/OfflineHQ/marketplace/commit/f278445d1547bff9be122222c344997d25296734)
⏱️ Estimated effort to review [1-5] | 4, because the PR includes a significant amount of changes across multiple files and systems, involving complex features like GraphQL type refactoring, integration tests, and feature flag management. The complexity and breadth of changes require thorough testing and understanding of multiple domains. |
🧪 Relevant tests | Yes |
🔍 Possible issues | Possible Bug: The `Posthog` class in `libs/insight/server/src/lib/insight-server.d.ts` has a very complex method `getFeatureFlagPayload` which could be prone to errors due to its deeply nested structures and multiple types. Simplifying this method could reduce potential bugs and improve maintainability. |
Performance Concern: The `loyalty-card-cron.integration.test.ts` file includes multiple test cases that mock external API calls. If these mocks are not accurately representing the API's behavior, it could lead to false positives in testing, where tests pass but the actual integration might fail under real conditions. | |
🔒 Security concerns | No |
Category | Suggestions |
Enhancement |
Add error handling for the Headers constructor to improve robustness.___ **Consider adding error handling for thenew Headers constructor to catch potential exceptions that might be thrown if the input is invalid.** [libs/integrations/external-api-handlers/src/lib/shopify/index.spec.ts [34-36]](https://github.com/OfflineHQ/marketplace/pull/302/files#diff-df93842e018141cf209891b7f7fa3c10fbc063d5eed17ace2701d2d12a234844R34-R36) ```diff -new Headers({ - 'x-shopify-client-id': 'validApiKey', -}) +try { + new Headers({ + 'x-shopify-client-id': 'validApiKey', + }) +} catch (error) { + console.error('Failed to create headers:', error); +} ``` |
Enhance the test coverage for
___
**To ensure that the | |
Add error handling and retry mechanism to the sign-in process.___ **TheloginSiwe function should handle the case where signIn fails due to network issues or server errors. Currently, it only logs the error but does not retry or provide a fallback mechanism. Implementing a retry mechanism or a fallback strategy can improve the robustness of the sign-in process.** [libs/next/auth/src/lib/safeAuthSetup.tsx [73-80]](https://github.com/OfflineHQ/marketplace/pull/302/files#diff-62c2e484cf5b18e47c237cc9a40b19276134b027875bfbf22bb37acb63d1194eR73-R80) ```diff const signInRes = await signIn('credentials', { message, redirect: false, signature, address, }); if (signInRes?.error) { console.error('Error signing in with SIWE:', signInRes?.error); + // Implement retry logic or fallback strategy here } ``` | |
Add error handling to the account creation process.___ **ThecreateAccount function currently does not handle exceptions that might be thrown by connectWithSiwe or loginSiwe . It is recommended to add error handling to manage these exceptions gracefully.** [libs/next/auth/src/lib/safeAuthSetup.tsx [179-181]](https://github.com/OfflineHQ/marketplace/pull/302/files#diff-62c2e484cf5b18e47c237cc9a40b19276134b027875bfbf22bb37acb63d1194eR179-R181) ```diff const createAccount = useCallback(async () => { - await connectWithSiwe(loginSiwe, undefined, true); + try { + await connectWithSiwe(loginSiwe, undefined, true); + } catch (error) { + console.error('Failed to create account:', error); + // Handle error appropriately + } }, [connectWithSiwe, loginSiwe]); ``` | |
Ensure complete logout process before resetting user tracking.___ **Thelogout function should verify if the disconnect and logoutSiwe operations are successful before calling logoutUserPostHog . This ensures that the user is fully logged out from all systems before resetting the tracking information.** [libs/next/auth/src/lib/safeAuthSetup.tsx [170-176]](https://github.com/OfflineHQ/marketplace/pull/302/files#diff-62c2e484cf5b18e47c237cc9a40b19276134b027875bfbf22bb37acb63d1194eR170-R176) ```diff const logout = useCallback( async ({ refresh }: { refresh?: boolean }) => { - await disconnect(); - await logoutSiwe({ refresh }); - logoutUserPostHog(); + try { + await disconnect(); + await logoutSiwe({ refresh }); + logoutUserPostHog(); + } catch (error) { + console.error('Failed to fully logout:', error); + // Handle incomplete logout scenario + } }, [disconnect, logoutSiwe, logoutUserPostHog], ); ``` | |
Improve the reliability of the connectivity check by handling potential errors.___ **TheisConnected function is used to check if the user is connected, but it is not clear how it handles different states of connectivity. It would be beneficial to add explicit handling for different connectivity states or errors that might occur during the connectivity check.** [libs/next/auth/src/lib/safeAuthSetup.tsx [190]](https://github.com/OfflineHQ/marketplace/pull/302/files#diff-62c2e484cf5b18e47c237cc9a40b19276134b027875bfbf22bb37acb63d1194eR190-R190) ```diff -const isNextAuthConnected = isConnected ? await isConnected() : false; +let isNextAuthConnected = false; +try { + isNextAuthConnected = isConnected ? await isConnected() : false; +} catch (error) { + console.error('Failed to check connectivity:', error); + // Handle connectivity check error +} ``` | |
Add error handling to the auto-login process to enhance stability.___ **TheloginAuto function attempts to log in automatically but does not handle the case where the connectWithSiwe or loginSiwe might fail. Adding error handling here would prevent unhandled promise rejections and improve the stability of the auto-login feature.** [libs/next/auth/src/lib/safeAuthSetup.tsx [152-162]](https://github.com/OfflineHQ/marketplace/pull/302/files#diff-62c2e484cf5b18e47c237cc9a40b19276134b027875bfbf22bb37acb63d1194eR152-R162) ```diff const loginAuto = useCallback( async (address: string) => { - if (!!session?.user && session?.user?.address !== address) { - await logoutSiwe({ refresh: false }); + try { + if (!!session?.user && session?.user?.address !== address) { + await logoutSiwe({ refresh: false }); + } + console.log('Auto login with SIWE...', address); + const instance = await connectWithSiwe(loginSiwe, address, true); + if (!instance) throw new Error('Error connecting with SIWE'); + else await loginSiwe(instance); + } catch (error) { + console.error('Failed to auto-login with SIWE:', error); + // Handle error during auto-login } - console.log('Auto login with SIWE...', address); - const instance = await connectWithSiwe(loginSiwe, address, true); - if (!instance) throw new Error('Error connecting with SIWE'); - else await loginSiwe(instance); }, [connectWithSiwe, loginSiwe, logoutSiwe, session], ); ``` | |
Use more specific matchers in test assertions for improved test accuracy.___ **Use a more specific matcher thanexpect.anything() in your test assertions to ensure that the function is called with the expected type of arguments, enhancing the test's accuracy and reliability.** [libs/features/loyalty-card-cron/src/lib/loyalty-card-cron.integration.test.ts [102]](https://github.com/OfflineHQ/marketplace/pull/302/files#diff-4f7b7cdfce4314f6e816b30477dcf2dae3f97994cdbcecdffc18863f89aaa74dR102-R102) ```diff -expect.anything(), +expect.any(Object), // or a more specific type if applicable ``` | |
Maintainability |
Centralize the creation of mock requests to reduce redundancy and improve code maintainability.___ **Refactor the repeated creation ofNextRequest objects in tests by centralizing the creation logic into a single function or utilizing a setup function to reduce redundancy and improve maintainability.** [libs/integrations/external-api-handlers/src/lib/shopify/index.spec.ts [172-179]](https://github.com/OfflineHQ/marketplace/pull/302/files#diff-df93842e018141cf209891b7f7fa3c10fbc063d5eed17ace2701d2d12a234844R172-R179) ```diff -const mockRequest = createMockRequest( - new URLSearchParams({ - password: 'test-password', - ownerAddress: 'test-address', - shop: 'example.myshopify.com', - timestamp: Date.now().toString(), - signature: 'validSignature', - }), -); +function createStandardMockRequest() { + return createMockRequest( + new URLSearchParams({ + password: 'test-password', + ownerAddress: 'test-address', + shop: 'example.myshopify.com', + timestamp: Date.now().toString(), + signature: 'validSignature', + }), + ); +} +const mockRequest = createStandardMockRequest(); ``` |
Refactor repeated mock setup into a utility function to reduce duplication.___ **Refactor the repeated mock setup foradminSdk.GetLoyaltyCardByContractAddressForProcess into a separate utility function to reduce code duplication and improve maintainability.** [libs/features/loyalty-card-cron/src/lib/loyalty-card-cron.integration.test.ts [69]](https://github.com/OfflineHQ/marketplace/pull/302/files#diff-4f7b7cdfce4314f6e816b30477dcf2dae3f97994cdbcecdffc18863f89aaa74dR69-R69) ```diff -adminSdk.GetLoyaltyCardByContractAddressForProcess as jest.Mock +setupMockGetLoyaltyCardByContractAddressForProcess(); ``` | |
Remove redundant test assertions or replace them with more specific checks.___ **Replace the redundant calls toexpect(adminSdk.GetLoyaltyCardByContractAddressForProcess).toHaveBeenCalled(); with a single call or use different assertions to validate different aspects of the mock's usage if needed.** [libs/features/loyalty-card-cron/src/lib/loyalty-card-cron.integration.test.ts [95-99]](https://github.com/OfflineHQ/marketplace/pull/302/files#diff-4f7b7cdfce4314f6e816b30477dcf2dae3f97994cdbcecdffc18863f89aaa74dR95-R99) ```diff expect( adminSdk.GetLoyaltyCardByContractAddressForProcess, -).toHaveBeenCalled(); +).toHaveBeenCalledTimes(1); ``` | |
Best practice |
Use a fixed or mocked timestamp in tests to ensure determinism.___ **Replace the direct usage ofDate.now() in tests with a mock or a fixed timestamp to make the tests deterministic and not dependent on the actual current time.** [libs/integrations/external-api-handlers/src/lib/shopify/index.spec.ts [123]](https://github.com/OfflineHQ/marketplace/pull/302/files#diff-df93842e018141cf209891b7f7fa3c10fbc063d5eed17ace2701d2d12a234844R123-R123) ```diff -timestamp: Date.now().toString(), +timestamp: 'fixed-timestamp', ``` |
Use specific error types for better context and debugging.___ **Use a more specific error type thanError for better error handling and to provide more context in the error messages, improving the debugging process.** [libs/integrations/external-api-handlers/src/lib/shopify/index.spec.ts [402]](https://github.com/OfflineHQ/marketplace/pull/302/files#diff-df93842e018141cf209891b7f7fa3c10fbc063d5eed17ace2701d2d12a234844R402-R402) ```diff -.mockRejectedValue(new Error('Unexpected error')), +.mockRejectedValue(new InternalServerError('Unexpected error')), ``` | |
Make test data generation deterministic to ensure test reliability and reproducibility.___ **Replace the direct use ofMath.random() in the generateNFTs function with a deterministic approach for generating test data. This change will make the tests more predictable and reproducible, which is crucial for debugging and reliability in automated testing environments.** [libs/features/loyalty-card-cron/src/lib/loyalty-card-cron.integration.test.ts [495]](https://github.com/OfflineHQ/marketplace/pull/302/files#diff-4f7b7cdfce4314f6e816b30477dcf2dae3f97994cdbcecdffc18863f89aaa74dR495-R495) ```diff -const numNFTs = Math.floor(Math.random() * 10) + 1; +const numNFTs = 5; // Fixed number for predictability in tests ``` | |
Add error handling for asynchronous operations in test setup and teardown.___ **Consider adding error handling for the asynchronous operations within thebeforeAll , afterAll , and beforeEach blocks to catch and handle any potential errors that might occur during database setup and teardown. This will improve the robustness of your test suite.** [libs/features/loyalty-card-cron/src/lib/loyalty-card-cron.integration.test.ts [35]](https://github.com/OfflineHQ/marketplace/pull/302/files#diff-4f7b7cdfce4314f6e816b30477dcf2dae3f97994cdbcecdffc18863f89aaa74dR35-R35) ```diff -await deleteAllTables(client); +try { + await deleteAllTables(client); +} catch (error) { + console.error('Error during table deletion:', error); +} ``` | |
Improve type safety by specifying a more precise type than
___
**Consider using a more specific type instead of | |
Enhance type safety by using specific types instead of
___
**Replace the | |
Improve type safety by specifying a more precise type for
___
**Use a specific type for the | |
Use a more specific type for the
___
**Specify a more precise type for the |
Type
enhancement, tests
Description
libs/gql/admin/types/src/generated/index.ts
to improve type safety and remove unused fields.Posthog
class inlibs/insight/server/src/lib/insight-server.d.ts
for managing feature flags with complex payload types.libs/features/loyalty-card-cron/src/lib/loyalty-card-cron.integration.test.ts
, covering multiple scenarios for NFT claims.libs/features/organizer/event/src/lib/organisms/PassList/PassList.stories.tsx
.initKyc
function.Changes walkthrough
index.ts
Refactor GraphQL Types for Account and Order Management
libs/gql/admin/types/src/generated/index.ts
scwAddress, email, and phone.
types for better type safety.
insight-server.d.ts
Add Posthog Class for Feature Flag Management
libs/insight/server/src/lib/insight-server.d.ts
and payloads.
feature flag payloads.
PassList.stories.tsx
Cleanup Unused Imports in PassList Stories
libs/features/organizer/event/src/lib/organisms/PassList/PassList.stories.tsx
stories setup.
index.ts
Export initKyc Function in KYC Actions Module
libs/features/kyc-actions/src/index.ts - Added export for initKyc function to the module's public API.
loyalty-card-cron.integration.test.ts
Implement Integration Tests for Loyalty Card Cron
libs/features/loyalty-card-cron/src/lib/loyalty-card-cron.integration.test.ts
single and multiple NFT claims.