Fix new created entity outlined

[Corantin]

• Fix new community/pool/proposal outlined
• Dont show "Pulling new data" toast when change detected from other client

[vercel]

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

Name	Status	Preview	Comments	Updated (UTC)
gardens-v2	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 1, 2024 5:51pm

[Corantin]

!ai

[github-actions]

Let's break down the pull request reviews and focus on the critical points:

Overall, the developer is making positive changes by migrating to context and improving subgraph queries. However, there are security and clarity issues that need to be addressed.

Pressing Issues:

• apps/web/lib/chains.ts - Security Risk:

  • Using localStorage and the sQueryAllChains global variable to determine which chains to query is highly insecure. A user could easily manipulate this to force queries on unintended chains.
  • Solution:
    • Use environment variables to control chain selection for different environments (development, production, etc.). This keeps the logic server-side and secure.
    • If user customization is needed, provide a UI setting and store the preference securely server-side.

• apps/web/hooks/useSubgraphQuery.ts - Clarity:

  • The retryCount recursion in this file is difficult to follow.
  • Solution: Refactor the retryCount logic to use a loop instead of recursion for better readability.

• apps/web/hooks/useSubgraphQueryMultiChain.ts - Incomplete Review:

  • The provided code snippet is incomplete, preventing a full assessment. The developer needs to provide the entire code for review.

Code Suggestions & Improvements:

• collectQueryParams.context.tsx:

  • Rename useCollectQueryParams to useQueryParams for brevity and consistency.

• apps/web/providers/Providers.tsx:

  • Add comments explaining the purpose and usage of QueryParamsProvider.
  • Consider if there's a more specific location for this provider closer to where it's used.

• General:

  • The developer should provide more context and examples of how the QueryParamsContext is used in the updated components.

To enhance this pull request, the developer should prioritize the security fix in chains.ts, improve code clarity in useSubgraphQuery.ts, and provide the missing code for useSubgraphQueryMultiChain.ts. Addressing the naming and commenting suggestions will further improve code maintainability.

## Review of Pull Request: Migrate `useCollectQueryParams` to Context

This pull request refactors the useCollectQueryParams hook into a context provider. This is a generally good practice for widely used hooks, especially those dealing with global state like query parameters.

Positive Changes:

• Improved Performance: Using a context can potentially improve performance by avoiding unnecessary re-renders. If the query parameters change frequently, components using the hook would re-render even if they are not directly affected by the specific parameter change. A context can help optimize this.
• Centralized Logic: Moving the logic into a context promotes better code organization and maintainability.
• Easier Testing: Context providers can be easily mocked and tested in isolation.

Potential Improvements:

• File Name: Consider renaming collectQueryParams.context.ts to something more descriptive, like queryParamsProvider.context.ts.
• Context Usage: While the PR moves the logic to a context, it's unclear how the context is used in the updated components. Ideally, you should provide examples of how these components consume the QueryParamsContext to leverage the query parameters.

File Specific Comments:

• apps/web/app/(app)/gardens/[chain]/[garden]/[community]/[poolId]/page.tsx:
  • No specific comments, as the change seems straightforward.
• apps/web/app/(app)/gardens/[chain]/[garden]/[community]/page.tsx:
  • No specific comments.
• apps/web/app/(app)/gardens/[chain]/[garden]/page.tsx:
  • No specific comments.
• apps/web/components/CommunityCard.tsx:
  • No specific comments.
• apps/web/components/PoolCard.tsx:
  • No specific comments.
• apps/web/components/ProposalCard.tsx:
  • Consider Consistency: While not a major issue, using both shadow-xl and shadow-2xl within the same component might affect visual consistency. Consider choosing one or using a design system's predefined shadow classes.

Review Summary:

This pull request introduces a good refactoring by migrating the useCollectQueryParams hook to a context provider. The change promotes better code organization and potential performance improvements. However, providing clearer context usage examples and addressing the minor naming and styling suggestions would further enhance this PR.

Review of Pull Request:

Summary:

This PR introduces a new context for collecting query parameters and removes the old hook. It also includes some improvements to the subgraph query hooks.

File by File:

• apps/web/contexts/collectQueryParams.context.tsx (NEW):
  • Good: This file provides a clean way to collect and access query parameters globally in the application.
  • Potential Improvement: Consider renaming useCollectQueryParams to useQueryParams for brevity and consistency with other naming conventions.
• apps/web/hooks/useCollectQueryParams.ts (DELETED):
  • Good: Removing the old hook simplifies the codebase and promotes consistency.
• apps/web/hooks/useSubgraphQuery.ts (MODIFIED):
  • Good: The addition of the withToast parameter to refetch provides better control over toast notifications during data fetching.
  • Minor Issue: The retryCount logic seems a bit convoluted. Consider using a loop instead of recursion for clarity.
• apps/web/hooks/useSubgraphQueryMultiChain.ts (MODIFIED):
  • Issue: The code snippet cuts off prematurely, preventing a complete review of the changes made in this file.

Overall:

The changes are well-structured and improve code organization. However, addressing the issues mentioned above will enhance code clarity and functionality.

Review of Code Changes

File: apps/web/lib/chains.ts

Issue: The code introduces a new global variable sQueryAllChains which is potentially dangerous and can lead to unexpected behavior. Relying on local storage for such a flag, especially without proper validation, can cause inconsistencies and potential security risks.

Security Implications:

• Data Integrity: A user could manipulate the local storage value, forcing the application to query all chains even in production.
• Untrusted Data: Local storage is client-side and can be easily tampered with. Using it to store a flag that dictates application behavior might lead to security vulnerabilities.

Suggestions:

• Use Environment Variables: A more secure and robust approach would be to use environment variables to control which chains are queried in different environments (development, production, etc.). This avoids relying on client-side data and provides better control.
• Explicit Configuration: If you need user-specific settings, provide a clear configuration option within the application (e.g., a toggle in the settings) and store this preference securely on the server-side if needed.
• Validation: Always validate data retrieved from local storage. Ensure the value is of the expected type and within acceptable bounds before using it.

File: apps/web/providers/Providers.tsx

Issue: The code adds QueryParamsProvider without a clear explanation of its purpose or usage. Adding providers without context can make the code harder to understand and maintain.

Suggestions:

• Add Comments: Briefly explain the purpose of QueryParamsProvider and how it interacts with other components.
• Consider Placement: If QueryParamsProvider is related to routing or specific components, consider placing it closer to where it's actually used.

Review Summary

The changes introduce potential security risks by relying on local storage for a critical flag. Using environment variables and providing explicit user configuration options would be a more secure and maintainable approach. Additionally, adding comments and context around the new provider would improve code readability and maintainability.

</details>