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

Feature/resolver #2923

Merged
merged 4 commits into from
Mar 1, 2025
Merged

Feature/resolver #2923

merged 4 commits into from
Mar 1, 2025

Conversation

DIYgod
Copy link
Member

@DIYgod DIYgod commented Mar 1, 2025

Description

PR Type

  • Feature
  • Bugfix
  • Hotfix
  • Other (please describe):

Screenshots (if UI change)

Demo Video (if new feature)

Linked Issues

Additional context

Changelog

  • I have updated the changelog/next.md with my changes.

Copy link

vercel bot commented Mar 1, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
follow ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 1, 2025 9:45am
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
follow-external-ssr ⬜️ Ignored (Inspect) Visit Preview Mar 1, 2025 9:45am

@follow-reviewer-bot
Copy link

Suggested PR Title:

refactor(env): streamline environment handling and API calls

Change Summary:
Refactor environment handling in mobile app and optimize API requests by utilizing centralized environment variables. Remove obsolete environment-related files and streamline code for better maintainability.

Code Review:

Code Review

The following issues require change requests:

  1. File: apps/mobile/metro.config.js

    • Lines: 8-9
      The new resolveRequest logic introduces a custom file resolution mechanism, but there is no validation if context.fileSystemLookup(mobilePath) returns an undefined or unexpected object. This could potentially lead to runtime errors. Add proper error handling and validation for the file.exists check.

    • Lines: 8-24
      The custom resolveRequest logic is modifying the resolution process without fallback or logging for debugging purposes in case of unexpected issues. Consider adding logging or fallback logic to make debugging easier.

  2. File: packages/shared/src/env.mobile.ts

    • Line: 2
      The variable profile is set to "prod" but hardcoded. This reduces flexibility for dynamically switching between environments. Consider externalizing this variable to be configurable at runtime (e.g., via environment variables or a .env file).
  3. File: apps/mobile/src/lib/image.ts

    • Line: 1
      The addition of import { imageRefererMatches } from "@follow/shared/src/image" results in direct dependency on the shared package. Ensure this shared structure is stable and does not introduce breaking changes for mobile. If there are frequent changes in this file, consider decoupling the mobile app from this dependency by introducing an adapter or other abstraction layer.
  4. File: apps/mobile/src/lib/api-fetch.ts

    • Line: 9
      Directly using env.VITE_API_URL reduces flexibility for testing or overriding the API URL at runtime. Consider adding a fallback mechanism for development and testing purposes.

    • Line: 21
      Replace env.VITE_API_URL with a consolidated utility function (if applicable) to allow consistent management of environment variables and future extensibility.

  5. File: apps/mobile/src/lib/auth.ts

    • Line: 14
      Similar to the api-fetch.ts file, the direct usage of env.VITE_API_URL reduces flexibility. Consider centralizing environment variable access via a helper function to ensure consistent use across the codebase.

Summary

These issues with error handling, hardcoded values, and limited flexibility for runtime configuration should be addressed to ensure a more robust and maintainable codebase.

@DIYgod DIYgod marked this pull request as ready for review March 1, 2025 10:31
@DIYgod DIYgod merged commit abd7f87 into dev Mar 1, 2025
10 of 11 checks passed
@DIYgod DIYgod deleted the feature/resolver branch March 1, 2025 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant