Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(native-app): Barcode fixes #17346

Merged
merged 18 commits into from
Jan 16, 2025
Merged

feat(native-app): Barcode fixes #17346

merged 18 commits into from
Jan 16, 2025

Conversation

thoreyjona
Copy link
Contributor

@thoreyjona thoreyjona commented Dec 23, 2024

What

Various barcode fixes:

  • Use cache-first fetch policy for licenses on home screen and license screen
  • New more compact design for licenses on home screen and license screen
  • Translate all license info when switching locale
  • Use name of license from server instead of hardcoded name
  • Add last updated and a refresh button to license screen
  • Add error state to license detail view if details view fails
  • Hide apple wallet note if barcode feature flag is enabled
  • Add time to last updated in license details view
  • Make sure to always show error state if something goes wrong (error or no connection)
  • Add max width on barcode to make it smaller in tablet sizes

TODO

  • OnPress on refresh button on ios is buggy - need to implement the ui side of the refresh manually

Screenshots

Screen.Recording.2024-12-23.at.07.53.18.mov

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Formatting passes locally with my changes
  • I have rebased against main before asking for a review

Summary by CodeRabbit

Release Notes

  • New Features

    • Added a name field to the metadata of user licenses.
    • Enhanced GetLicense query to support locale-specific responses.
    • Introduced new entries for wallet and license-related messages, improving user feedback and error handling.
    • Added LicenseListCard component for displaying license information.
    • Implemented a refresh button and last updated timestamp in the Wallet screen.
    • Adjusted barcode dimensions for tablet devices and improved localization support in the Wallet Pass screen.
  • Bug Fixes

    • Improved error handling and loading states in the LicenseCard component.
  • Chores

    • Updated localization strings for improved clarity and functionality.
    • Enhanced data-fetching strategy for licenses to prioritize cached data.

@thoreyjona thoreyjona requested a review from a team as a code owner December 23, 2024 07:52
Copy link
Contributor

coderabbitai bot commented Dec 23, 2024

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 eslint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options.
warning eslint > @humanwhocodes/[email protected]: Use @eslint/config-array instead
warning eslint > @humanwhocodes/config-array > @humanwhocodes/[email protected]: Use @eslint/object-schema instead
warning eslint > file-entry-cache > flat-cache > [email protected]: Rimraf versions prior to v4 are no longer supported
warning eslint > file-entry-cache > flat-cache > rimraf > [email protected]: Glob versions prior to v9 are no longer supported
warning eslint > file-entry-cache > flat-cache > rimraf > glob > [email protected]: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
warning jest > @jest/core > jest-config > [email protected]: Glob versions prior to v9 are no longer supported
warning jest > @jest/core > @jest/reporters > [email protected]: Glob versions prior to v9 are no longer supported
warning jest > @jest/core > jest-runtime > [email protected]: Glob versions prior to v9 are no longer supported
warning jest > @jest/core > @jest/transform > babel-plugin-istanbul > test-exclude > [email protected]: Glob versions prior to v9 are no longer supported
warning storybook > @storybook/cli > puppeteer-core > [email protected]: Rimraf versions prior to v4 are no longer supported
warning storybook > @storybook/cli > puppeteer-core > rimraf > [email protected]: Glob versions prior to v9 are no longer supported
warning storybook > @storybook/cli > jscodeshift > temp > [email protected]: Rimraf versions prior to v4 are no longer supported
warning storybook > @storybook/cli > jscodeshift > temp > rimraf > [email protected]: Glob versions prior to v9 are no longer supported
warning storybook > @storybook/cli > jscodeshift > @babel/[email protected]: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-optional-chaining instead.
warning storybook > @storybook/cli > jscodeshift > @babel/[email protected]: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-class-properties instead.
warning storybook > @storybook/cli > tempy > del > [email protected]: Rimraf versions prior to v4 are no longer supported
warning storybook > @storybook/cli > jscodeshift > @babel/[email protected]: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-nullish-coalescing-operator instead.
warning react-native > @react-native/codegen > [email protected]: Glob versions prior to v9 are no longer supported
warning react-native > @react-native/community-cli-plugin > [email protected]: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.
warning react-native > @react-native-community/cli > @react-native-community/cli-tools > [email protected]: Package no longer supported. Contact Support at https://www.npmjs.com/support for more info.
warning react-native > @react-native/community-cli-plugin > @react-native/dev-middleware > @rnx-kit/chromium-edge-launcher > [email protected]: Rimraf versions prior to v4 are no longer supported
warning react-native > @react-native/community-cli-plugin > @react-native/metro-babel-transformer > @react-native/babel-preset > @babel/[email protected]: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-class-properties instead.
warning react-native > @react-native/community-cli-plugin > @react-native/metro-babel-transformer > @react-native/babel-preset > @babel/[email protected]: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-optional-chaining instead.
warning react-native > @react-native/community-cli-plugin > @react-native/metro-babel-transformer > @react-native/babel-preset > @babel/[email protected]: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-nullish-coalescing-operator instead.
warning react-native > @react-native/community-cli-plugin > @react-native/metro-babel-transformer > @react-native/babel-preset > @babel/[email protected]: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-object-rest-spread instead.
warning react-native > @react-native/community-cli-plugin > @react-native/metro-babel-transformer > @react-native/babel-preset > @babel/[email protected]: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-numeric-separator instead.
warning react-native > @react-native/community-cli-plugin > @react-native/metro-babel-transformer > @react-native/babel-preset > @babel/[email protected]: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-optional-catch-binding instead.
warning react-native > @react-native/community-cli-plugin > @react-native/metro-babel-transformer > @react-native/babel-preset > @babel/[email protected]: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-async-generator-functions instead.
warning react-native > @react-native/community-cli-plugin > @react-native/metro-babel-transformer > @react-native/babel-preset > @babel/[email protected]: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-logical-assignment-operators instead.
warning next-auth > [email protected]: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.
warning next-auth > [email protected]: this version is no longer supported
warning next-auth > @next-auth/typeorm-legacy-adapter > typeorm > [email protected]: Glob versions prior to v9 are no longer supported
warning @nx/next > @nx/webpack > stylus > [email protected]: Glob versions prior to v9 are no longer supported
warning @nx/next > @nx/webpack > webpack-dev-server > [email protected]: Rimraf versions prior to v4 are no longer supported
warning @nx/next > @nx/webpack > fork-ts-checker-webpack-plugin > [email protected]: this will be v4
warning @nx/next > @nx/webpack > webpack-dev-server > webpack-dev-middleware > [email protected]: this will be v4
warning workspace-aggregator-278dcb5e-53fe-4934-890c-19795b26d135 > [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options.
warning "@nx/eslint > @nx/js > [email protected]" has unmet peer dependency "@types/node@".
warning "@nx/next > @babel/[email protected]" has unmet peer dependency "@babel/core@^7.0.0-0".
warning "styled-components > babel-plugin-styled-components > @babel/[email protected]" has unmet peer dependency "@babel/core@^7.0.0-0".
warning " > [email protected]" has unmet peer dependency "react-is@>= 16.8.0".
warning "@nx/react > [email protected]" has unmet peer dependency "webpack@^4.0.0 || ^5.0.0".
warning " > [email protected]" has unmet peer dependency "@types/node@
".
warning " > [email protected]" has incorrect peer dependency "[email protected]".
warning "react-native > @react-native/[email protected]" has unmet peer dependency "@babel/preset-env@^7.1.6".
warning "react-native > @react-native/community-cli-plugin > @react-native/[email protected]" has unmet peer dependency "@babel/core@*".
warning "@vanilla-extract/next-plugin > @vanilla-extract/[email protected]" has unmet peer dependency "webpack@^4.30.0 || ^5.20.2".
warning " > [email protected]" has incorrect peer dependency "react@^16.13.1 || ^17".
warning " > [email protected]" has incorrect peer dependency "react-dom@^16.13.1 || ^17".
warning "next-auth > @next-auth/[email protected]" has unmet peer dependency "@prisma/client@^2.16.1".
warning "@nx/next > [email protected]" has unmet peer dependency "webpack@^5.1.0".

Walkthrough

This pull request introduces enhancements to the license management functionality in the native application. The changes include adding a name field to the GenericUserLicenseFragment, updating GraphQL queries to support locale-specific responses, expanding localization messages, and modifying various components to handle new license-related features. The modifications improve the wallet and license display with more detailed information, better error handling, and enhanced localization support.

Changes

File Change Summary
apps/native/app/src/graphql/fragments/license.fragment.graphql Added name field to metadata in GenericUserLicenseFragment
apps/native/app/src/graphql/queries/licenses.graphql Updated GetLicense query to include $locale parameter
apps/native/app/src/messages/en.ts Added new localization entries for wallet and license-related messages
apps/native/app/src/messages/is.ts Added Icelandic translations for new wallet and license messages
apps/native/app/src/screens/home/licenses-module.tsx Added fetchPolicy: 'cache-first' to useListLicensesQuery
apps/native/app/src/screens/wallet-pass/wallet-pass.tsx Added locale support, barcode width handling, and enhanced LicenseCard props
apps/native/app/src/screens/wallet/components/wallet-item.tsx Replaced LicenseCard with LicenseListCard
apps/native/app/src/screens/wallet/wallet.tsx Added last updated timestamp, refresh button, and feature flag handling
apps/native/app/src/ui/index.ts Added export for license-list-card component
apps/native/app/src/ui/lib/card/license-card.tsx Updated to support loading and error states
apps/native/app/src/ui/lib/card/license-list-card.tsx New component for rendering license list cards

Sequence Diagram

sequenceDiagram
    participant User
    participant WalletScreen
    participant LicenseQuery
    participant LocaleService
    
    User->>WalletScreen: Open Wallet
    WalletScreen->>LocaleService: Get Current Locale
    LocaleService-->>WalletScreen: Return Locale
    WalletScreen->>LicenseQuery: Fetch Licenses with Locale
    LicenseQuery-->>WalletScreen: Return License Data
    WalletScreen->>WalletScreen: Render License List
Loading

Suggested labels

deploy-feature

Suggested reviewers

  • thordurhhh
  • eirikurn

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (7)
apps/native/app/src/screens/home/licenses-module.tsx (1)

78-78: cache-first policy may result in stale data.
While "cache-first" can enhance performance, consider situations where fresh data is critical. Using "cache-and-network" or manually invalidating the cache might better serve users needing the latest license updates.

apps/native/app/src/ui/lib/card/license-list-card.tsx (1)

36-46: Potential React Native row-gap caution.
React Native’s support for the "row-gap" property can be inconsistent across platforms. Ensure that your targeted platforms support row-gap as expected, or consider alternative spacing approaches if you encounter layout issues.

apps/native/app/src/ui/lib/card/license-card.tsx (1)

86-87: Use of generic props in styled component.
Defining the color prop provides flexibility, but be sure to handle default/fallback values if color is undefined.

apps/native/app/src/screens/wallet/wallet.tsx (2)

155-160: Retrieving and formatting “lastUpdated.”
This approach surfaces timely info to users, though consider including time of update or using FormattedDateTime to show both date & time.


305-328: Footer content for last update and refresh.

  1. Good user experience: showing “last updated” along with a manual refresh option.
  2. Consider a more declarative approach to handle success/error states from refetch (e.g., a toast or inline message).
apps/native/app/src/messages/is.ts (1)

406-412: Double-check error messaging and consider uniform translation for clarity.
The error messages and license state strings are clear and helpful. However, ensure all potential user-facing errors, including network and fetch failures, follow a similar pattern to maintain consistency.

apps/native/app/src/graphql/fragments/license.fragment.graphql (1)

44-44: Validate the new 'name' field usage.
This added field should be typed correctly and consistently handled in the UI. Consider adding a fallback if 'name' is null or undefined in the data.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e0a1eec and 6ae9f66.

⛔ Files ignored due to path filters (6)
  • apps/native/app/src/assets/icons/refresh.png is excluded by !**/*.png
  • apps/native/app/src/assets/icons/[email protected] is excluded by !**/*.png
  • apps/native/app/src/assets/icons/[email protected] is excluded by !**/*.png
  • apps/native/app/src/ui/assets/card/p-card.png is excluded by !**/*.png
  • apps/native/app/src/ui/assets/card/[email protected] is excluded by !**/*.png
  • apps/native/app/src/ui/assets/card/[email protected] is excluded by !**/*.png
📒 Files selected for processing (11)
  • apps/native/app/src/graphql/fragments/license.fragment.graphql (1 hunks)
  • apps/native/app/src/graphql/queries/licenses.graphql (1 hunks)
  • apps/native/app/src/messages/en.ts (2 hunks)
  • apps/native/app/src/messages/is.ts (2 hunks)
  • apps/native/app/src/screens/home/licenses-module.tsx (1 hunks)
  • apps/native/app/src/screens/wallet-pass/wallet-pass.tsx (5 hunks)
  • apps/native/app/src/screens/wallet/components/wallet-item.tsx (3 hunks)
  • apps/native/app/src/screens/wallet/wallet.tsx (8 hunks)
  • apps/native/app/src/ui/index.ts (1 hunks)
  • apps/native/app/src/ui/lib/card/license-card.tsx (10 hunks)
  • apps/native/app/src/ui/lib/card/license-list-card.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
apps/native/app/src/ui/index.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/screens/home/licenses-module.tsx (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/graphql/fragments/license.fragment.graphql (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/screens/wallet/components/wallet-item.tsx (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/graphql/queries/licenses.graphql (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/screens/wallet-pass/wallet-pass.tsx (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/messages/en.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/ui/lib/card/license-list-card.tsx (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/screens/wallet/wallet.tsx (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/ui/lib/card/license-card.tsx (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/messages/is.ts (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (36)
apps/native/app/src/ui/index.ts (1)

17-17: Export statement is consistent and expands library usage.
This addition seamlessly integrates the new "LicenseListCard" component into the global UI exports, ensuring consistency with other export statements in the file.

apps/native/app/src/screens/wallet/components/wallet-item.tsx (3)

5-5: Import statement aligns with updated exports.
Importing "LicenseListCard" from '../../../ui' is correct and follows the newly added export in "ui/index.ts".


56-59: LicenseListCard usage in Passport flow looks correct.
By passing "CustomLicenseType.Passport" and "licenseNumber" with a fallback, you ensure proper rendering for passports. Verify any locale-based text transformations if needed.


84-89: LicenseListCard usage for GenericUserLicense is appropriate.
Switching from the old LicenseCard to LicenseListCard makes sense. The new props (title, licenseNumber) correctly match the updated component interface, improving clarity.

apps/native/app/src/ui/lib/card/license-list-card.tsx (3)

1-14: Imports and utility usage are well-organized.
Using "react-intl" to localize content and "is-string"/"prefix-base-64" utilities is a good approach for flexible logo handling.


93-173: Presets pattern is clear and maintainable.
The "LicenseCardPresets" object effectively centralizes styling and brand assets for each license type. This is a clean approach, reducing duplication and making it simple to adjust branding in the future.


202-275: Well-structured “LicenseListCard” component.
• The design pattern (using presets, background images, and conditional base64 logos) is cohesive.
• The fallback to DriversLicense preset is reasonable, though consider explicit handling or warnings for unrecognized types.
• The localized rendering of passport vs. license number is a nice detail.

apps/native/app/src/ui/lib/card/license-card.tsx (11)

2-2: Good integration of react-intl.
These imports ensure proper localization of date/time, which complies with i18n best practices.


43-43: Max width usage for the barcode container.
Limiting the barcode’s container width to 500px helps prevent oversizing on larger screens, improving layout consistency.


77-77: Minor styling update.
This small margin-bottom change looks consistent with the theme spacing usage.


93-93: Ensure readability.
This styling property is coherent. No immediate issues spotted.


123-124: Optional props for loading and error states.
This is a clear approach that improves external control of component states.


143-144: Destructuring props for loading and error.
Well-structured approach to keep the code self-describing.


175-180: Heading variant usage.
Using “heading5” consistently with the design system helps maintain typography standards.


185-225: Conditionally rendering skeleton vs. status icons.

  1. Great use of a skeleton loader to handle loading states.
  2. Good approach of toggling between verified/non-verified icons based on “error” or “status.”

254-254: Conditional hiding of barcode.
This ensures the barcode is only displayed when not offline and no error state exists, improving user feedback clarity.


289-289: Fallback UI for offline/error states.
Effectively communicates potential connectivity or data fetch problems to the user.


296-298: Structured messages for different error states.
Nice reuse of react-intl IDs to differentiate between fetch errors and connectivity problems.

apps/native/app/src/screens/wallet/wallet.tsx (9)

16-23: Clean UI imports.
Centralized UI elements (e.g., Alert, Button, Typography) promote reusability and visual consistency in screens.


25-25: Refresh icon import.
No issues found; ensures a consistent UI for the refresh functionality.


41-42: Feature flag and locale usage.
Combining feature flags with localized queries is beneficial for a flexible, region-based user experience.


109-109: Flag-based barcode logic.
Storing the “isBarcodeEnabled” state in a dedicated feature flag is a clean approach, isolating experimental or controlled features.


126-126: Added locale to license query.
Ensures localized data is fetched. Great alignment with multilingual requirements.


128-128: Cache-first strategy
Leverages Apollo’s caching correctly, reducing network overhead while still providing up-to-date data on refetch.


206-206: GeneralCardSkeleton dimension adjustment.
Lowering its height from 104 to 80 helps ensure a compact skeleton, refining the placeholder layout.


Line range hint 252-253: Referenced lines not found in diff.
This might be an AI summary mention or a mismatch with the final code. No visible anomalies in the snippet.


277-277: Conditional rendering based on isBarcodeEnabled.
Effective approach for toggling user guidance or disclaimers. Keep an eye on potential re-renders if the flag changes frequently.

✅ Verification successful

Let me gather more information about how this feature flag is used in the components to better assess its impact on rendering.


Feature flag usage is well-implemented with minimal re-render impact
The feature flag is used appropriately with proper memoization and controlled re-renders. The useFeatureFlag hook is called at the component level, and its value primarily affects:

  • Conditional UI rendering for barcode display
  • Layout adjustments through a controlled re-render using a key state
  • Button visibility and positioning

The implementation includes proper handling of loading states and transitions, with explicit comments about temporary animation logic to prevent render flicker during flag initialization.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Using RG to locate "isBarcodeEnabled" across the codebase and validate if it triggers frequent re-renders
rg -A 3 "useFeatureFlag\('isBarcodeEnabled'" 

Length of output: 799


Script:

#!/bin/bash
# Check the usage context of isBarcodeEnabled in both files
rg -B 3 -A 10 "isBarcodeEnabled" apps/native/app/src/screens/wallet/wallet.tsx apps/native/app/src/screens/wallet-pass/wallet-pass.tsx

Length of output: 14486

apps/native/app/src/screens/wallet-pass/wallet-pass.tsx (5)

40-40: Locale-based data fetching
The addition of the useLocale() hook ensures the license info is correctly translated.


139-140: Constant for barcode max width
This is a clear approach to keep barcodes from becoming unwieldy on larger screens.


162-162: Added locale to GetLicense query
Aligns well with the rest of the PR’s emphasis on localization improvements.


175-177: Conditionally limiting barcode width.
Allows a more tablet-friendly layout, good for multi-device UX.


402-404: Passing loading and error to LicenseCard
Creates a consistent experience in the display logic and is easy to maintain.

apps/native/app/src/messages/en.ts (2)

252-253: New wallet last updated messages.
These additions let users see the last update time and trigger a manual refresh, enhancing transparency.


269-277: Refined walletPass error strings & license statuses.
These new message keys provide more meaningful user feedback, improving clarity for scanning behavior and license validity states.

apps/native/app/src/messages/is.ts (1)

388-389: Ensure consistent usage of date placeholders and translation keys.
These new localization keys appear correct, but please confirm that all usage sites supply the {date} parameter.

apps/native/app/src/graphql/queries/licenses.graphql (1)

7-8: Confirm usage of '$locale' argument.
Passing an optional locale parameter is a good improvement for localization, but ensure that all resolvers and client calls handle a null or missing locale gracefully.

✅ Verification successful

Let me search for the resolver implementation to verify how the locale parameter is handled.


Let me try a different approach to find the resolver implementation.


Let me check how the locale is being used in the client code.


Let me check how the locale is being used in the wallet-pass screen where the GetLicense query is being used.


Let me try a different approach to find the wallet-pass implementation and locale handling.


Let me try one final search to find the implementation.


Let me check the wallet-pass implementation since we found its location.


From examining the wallet-pass implementation, I can see that the locale is properly handled. The useLocale() hook is used to get the current locale, which is then passed to the useGetLicenseQuery. The locale is optional in the query, and the client code correctly supplies it. Let me check the schema definition to confirm the resolver side.


Let me check the resolver implementation since we found its location.


Based on the review of the implementation, I can now provide a final response:

Locale parameter is properly handled throughout the codebase
The locale parameter is well-implemented and safely handled:

  • In the GraphQL query, it's correctly defined as an optional String parameter
  • The client-side (wallet-pass.tsx) properly supplies the locale using the useLocale() hook
  • The resolver (UserLicenseResolver) has a default fallback to 'is' (Icelandic) when no locale is provided
  • The locale is passed through to the license service for localized content generation
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Searching for references to 'GetLicense' usage to ensure locale is being properly supplied or handled.
rg -A 10 "GetLicense" 

Length of output: 15563


Script:

#!/bin/bash
# Search for genericLicense resolver implementation
ast-grep --pattern 'genericLicense($$$) {
  $$$
}'

Length of output: 52


Script:

#!/bin/bash
# Search for files containing genericLicense resolver
rg -l "genericLicense.*resolver"

# Also search for any file that might handle the locale parameter
rg -A 5 "genericLicense.*locale"

Length of output: 5070


Script:

#!/bin/bash
# Search for useLocale hook implementation and usage
rg -A 5 "useLocale"

# Search for how the locale is handled in wallet-pass screen
rg -A 10 "useGetLicenseQuery.*locale"

Length of output: 65919


Script:

#!/bin/bash
# Search for the implementation of useLocale hook and how it's used in wallet-pass
rg -A 10 "src/hooks/use-locale"

# Search for the wallet-pass screen implementation
rg -A 10 "useGetLicenseQuery.*wallet-pass"

Length of output: 76


Script:

#!/bin/bash
# Search for the wallet-pass screen implementation and how it uses the locale
rg -A 10 "wallet-pass.tsx"

# Search for the genericLicense resolver implementation
rg -A 10 "genericLicense.*resolver"

Length of output: 64


Script:

#!/bin/bash
# Search for files containing wallet-pass
fd wallet-pass

# Search for the implementation of the resolver
rg -A 10 "type.*GenericLicenseResolver"

Length of output: 264


Script:

#!/bin/bash
# Check the implementation of wallet-pass screen
cat apps/native/app/src/screens/wallet-pass/wallet-pass.tsx

# Also check for any GraphQL schema definitions related to genericLicense
rg -A 5 "type.*GenericLicense.*{" 

Length of output: 15512


Script:

#!/bin/bash
# Search for GraphQL schema definition with fixed pattern
rg "type GenericLicense"

# Also check for any GraphQL input type definitions
rg "input.*GenericLicense"

Length of output: 2093


Script:

#!/bin/bash
# Check the resolver implementation
cat libs/api/domains/license-service/src/lib/resolvers/userLicense.resolver.ts

# Also check for any locale-related types or interfaces
rg -A 5 "interface.*License.*{|type.*License.*{"

Length of output: 2673

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
apps/native/app/src/screens/wallet-pass/wallet-pass.tsx (2)

167-167: Consider extracting tablet-specific logic

The tablet detection and dimension calculations are scattered across the component. Consider centralizing this logic for better maintainability.

Consider creating a custom hook:

const useTabletLayout = () => {
  const isTablet = screenWidth > 760;
  const getBarcodeWidth = (spacing: number) => 
    isTablet ? BARCODE_MAX_WIDTH : screenWidth - spacing;
  return { isTablet, getBarcodeWidth };
};

Also applies to: 174-176


401-403: Consider enhancing error handling

While the basic error state is implemented, consider adding more specific error handling for different failure scenarios (network errors, server errors, etc.).

Consider expanding the error prop to include more details:

error={{
  type: res.error?.networkError ? 'network' : 'server',
  message: res.error?.message,
  retryable: !res.error?.networkError
}}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6ae9f66 and 9ca7d16.

📒 Files selected for processing (2)
  • apps/native/app/src/screens/wallet-pass/wallet-pass.tsx (4 hunks)
  • apps/native/app/src/ui/lib/card/license-card.tsx (11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/native/app/src/ui/lib/card/license-card.tsx
🧰 Additional context used
📓 Path-based instructions (1)
apps/native/app/src/screens/wallet-pass/wallet-pass.tsx (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (1)
apps/native/app/src/screens/wallet-pass/wallet-pass.tsx (1)

40-43: LGTM! Well-structured additions for localization and responsive design

The new imports and constants effectively support the localization requirements and tablet-specific UI improvements.

Copy link
Member

@snaerth snaerth left a comment

Choose a reason for hiding this comment

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

Awesome work. This looks much better now 👏👏👏

apps/native/app/src/screens/wallet/wallet.tsx Outdated Show resolved Hide resolved
apps/native/app/src/screens/wallet/wallet.tsx Outdated Show resolved Hide resolved
apps/native/app/src/screens/wallet/wallet.tsx Show resolved Hide resolved
apps/native/app/src/ui/lib/card/license-card.tsx Outdated Show resolved Hide resolved
@Herdismaria Herdismaria added the deprecated:automerge (Disabled) Merge this PR as soon as all checks pass label Jan 15, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (6)
apps/native/app/src/ui/lib/card/license-list-card.tsx (4)

85-91: Fix alignment property in ImgWrap styled component.

The align-content property is incorrect here as it's used for multi-line flex containers. For centering a single item, use align-items.

 const ImgWrap = styled.View`
   flex-shrink: 0;
-  align-content: center;
+  align-items: center;
   justify-content: center;
   height: 42px;
   width: 42px;
 `

93-172: Consider internationalizing preset titles.

The titles in the presets are hardcoded in Icelandic. Consider using translation keys for better internationalization support.

Example refactor for one preset:

 DriversLicense: {
-  title: 'Ökuskírteini (IS)',
+  title: intl.formatMessage({ id: 'licenses.driversLicense.title' }),
   logo: LogoCoatOfArms,
   // ...
 }

219-219: Consider using theme-based text color calculation.

The textColor assignment could be more dynamic based on the background color to ensure proper contrast.

- const textColor = theme.shades.light.foreground
+ const textColor = backgroundColor 
+   ? theme.utils.getContrastColor(backgroundColor)
+   : theme.shades.light.foreground

263-271: Extract chevron styling to a styled component.

Move the inline styles to a styled component for better maintainability and reusability.

+ const ChevronIcon = styled(Image)`
+   height: 24px;
+   width: 24px;
+   tint-color: ${({ theme }) => theme.color.dark400};
+   opacity: 0.3;
+ `

- <Image
-   source={chevronForward}
-   style={{
-     height: 24,
-     width: 24,
-     tintColor: theme.color.dark400,
-     opacity: 0.3,
-   }}
- />
+ <ChevronIcon source={chevronForward} />
apps/native/app/src/screens/wallet/wallet.tsx (2)

164-164: Remove debug console.log statement.

Remove the console.log statement as it was likely used for debugging purposes.

-  console.log(lastUpdatedFormatted)

202-215: Extract magic numbers into named constants.

The scroll offset values (-300, -150) should be extracted into named constants for better maintainability and clarity.

+ const INITIAL_SCROLL_OFFSET = -300
+ const FINAL_SCROLL_OFFSET = -150

  const programmaticScrollWhenRefreshing = () => {
-   flatListRef.current?.scrollToOffset({ offset: -300, animated: true })
+   flatListRef.current?.scrollToOffset({ offset: INITIAL_SCROLL_OFFSET, animated: true })
    res
      .refetch()
      .then(() => {
-       flatListRef.current?.scrollToOffset({ offset: -150, animated: true })
+       flatListRef.current?.scrollToOffset({ offset: FINAL_SCROLL_OFFSET, animated: true })
      })
      .catch(() => {
-       flatListRef.current?.scrollToOffset({ offset: -150, animated: true })
+       flatListRef.current?.scrollToOffset({ offset: FINAL_SCROLL_OFFSET, animated: true })
      })
  }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 50828e0 and eaf108e.

📒 Files selected for processing (5)
  • apps/native/app/src/messages/en.ts (2 hunks)
  • apps/native/app/src/screens/wallet/components/wallet-item.tsx (3 hunks)
  • apps/native/app/src/screens/wallet/wallet.tsx (9 hunks)
  • apps/native/app/src/ui/lib/card/license-card.tsx (11 hunks)
  • apps/native/app/src/ui/lib/card/license-list-card.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • apps/native/app/src/screens/wallet/components/wallet-item.tsx
  • apps/native/app/src/ui/lib/card/license-card.tsx
  • apps/native/app/src/messages/en.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/native/app/src/screens/wallet/wallet.tsx (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/ui/lib/card/license-list-card.tsx (1)

Pattern apps/**/*: "Confirm that the code adheres to the following:

  • NextJS best practices, including file structure, API routes, and static generation methods.
  • Efficient state management and server-side rendering techniques.
  • Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (4)
apps/native/app/src/ui/lib/card/license-list-card.tsx (2)

1-34: LGTM! Well-organized imports and constant definition.

The imports are logically grouped, and the constant provides consistent spacing across the component.


202-202: Consider renaming the component to better reflect its purpose.

The component represents a single license card item rather than a list of licenses. Consider renaming it to LicenseItemCard for better clarity.

apps/native/app/src/screens/wallet/wallet.tsx (2)

126-128: LGTM! Cache-first implementation and locale support.

The implementation correctly adds locale support and implements the cache-first fetch policy as specified in the PR objectives.


327-331: Use styled-components instead of inline styling.

As previously suggested, prefer using styled-components over inline styles for better maintainability and consistency.

+ const FooterContainer = styled.View`
+   justify-content: center;
+   align-items: center;
+   margin-top: ${({ theme }) => theme.spacing[1]}px;
+ `

- <View
-   style={{
-     justifyContent: 'center',
-     alignItems: 'center',
-     marginTop: theme.spacing[1],
-   }}
- >
+ <FooterContainer>

apps/native/app/src/screens/wallet/wallet.tsx Show resolved Hide resolved
@Herdismaria Herdismaria added the autoupdate Branch gets auto updated label Jan 16, 2025
@kodiakhq kodiakhq bot merged commit 7e8309f into main Jan 16, 2025
31 of 40 checks passed
@kodiakhq kodiakhq bot deleted the feat/lazy-load-licenses branch January 16, 2025 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autoupdate Branch gets auto updated deprecated:automerge (Disabled) Merge this PR as soon as all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants