refactor(web): use lang at api hooks #1247
Conversation
WalkthroughThe changes in this pull request primarily involve the modification of various hooks and functions across multiple files to streamline the handling of language context in API queries. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (13)
web/src/beta/features/Editor/hooks/useScene.ts (1)
Line range hint
4-7: Remove unusedlangproperty fromScenePropstype.The
langproperty is defined in the type but not used in the implementation. This could mislead other developers into thinking that the hook accepts a language parameter.type SceneProps = { sceneId?: string; - lang?: string; };web/src/services/api/sceneApi.ts (2)
Line range hint
29-33: Remove unusedlangparameter from SceneQueryPropsThe
langparameter inSceneQueryPropsis no longer used since the language is now handled internally via theuseLanghook. This aligns with the PR's objective of standardizing language handling at the API level.export type SceneQueryProps = { sceneId?: string; - lang?: string; pollInterval?: number; };
57-87: LGTM! Good implementation of centralized language handlingThe changes effectively standardize the language handling at the API level by:
- Using the
useLanghook to centrally manage the language state- Including
langin the GraphQL query variables- Properly adding
langto theuseCallbackdependenciesThis implementation will help improve Apollo cache efficiency by ensuring consistent query structures.
Consider applying this pattern to other API hooks to maintain consistency across the codebase.
web/src/services/api/layerStyleApi/index.ts (2)
Line range hint
34-48: Consider memoizing the query variables objectWhile the implementation correctly uses the language context, the query variables object is recreated on every render. Consider memoizing it to prevent unnecessary re-renders:
const useGetLayerStylesQuery = useCallback( ({ sceneId }: LayerStylesQueryProps) => { + const variables = useMemo(() => ({ + sceneId: sceneId ?? "", + lang + }), [sceneId, lang]); const { data, loading, networkStatus, fetchMore, ...rest } = useQuery( GET_SCENE, { - variables: { sceneId: sceneId ?? "", lang }, + variables, skip: !sceneId } );
Apollo cache does not handle the
langparameter in cache policies. Please update the cache configuration to includelanginkeyFieldsortypePoliciesto ensure proper caching behavior.🔗 Analysis chain
Line range hint
34-48: Verify Apollo cache behavior with the new lang handlingLet's verify that the Apollo cache is working as expected with the standardized lang handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other GraphQL queries that might be affected by this change echo "Searching for GraphQL queries that use the lang parameter..." rg -l "lang.*:.*String" --type ts # Look for potential cache configuration echo "Checking Apollo cache configuration..." rg -l "InMemoryCache|cacheConfig" --type ts # Check for other usages of useGetLayerStylesQuery echo "Checking usages of useGetLayerStylesQuery..." rg "useGetLayerStylesQuery.*lang" --type tsLength of output: 583
Script:
#!/bin/bash # Extract instances where 'lang' is referenced in the Apollo cache configuration echo "Extracting 'lang' references in Apollo cache configuration..." rg "lang" web/src/services/gql/provider/index.tsx # Check for cache key modifications or type policies involving 'lang' echo "Checking for cache key modifications or type policies involving 'lang'..." rg -E "keyFields|typePolicies" web/src/services/gql/provider/index.tsxLength of output: 484
Script:
#!/bin/bash # Extract instances where 'lang' is referenced in the Apollo cache configuration echo "Extracting 'lang' references in Apollo cache configuration..." rg "lang" web/src/services/gql/provider/index.tsx # Check for cache key modifications or type policies involving 'lang' echo "Checking for cache key modifications or type policies involving 'lang'..." rg -E "keyFields|typePolicies" web/src/services/gql/provider/index.tsxLength of output: 484
Script:
#!/bin/bash # Extract instances where 'lang' is referenced in the Apollo cache configuration echo "Extracting 'lang' references in Apollo cache configuration..." rg "lang" web/src/services/gql/provider/index.tsx # Check for cache key modifications involving 'lang' echo "Checking for cache key modifications involving 'lang'..." rg 'keyFields' web/src/services/gql/provider/index.tsx # Check for type policies involving 'lang' echo "Checking for type policies involving 'lang'..." rg 'typePolicies' web/src/services/gql/provider/index.tsxLength of output: 559
web/src/services/api/storytellingApi/index.ts (2)
30-30: LGTM: Centralized language context managementGood practice to initialize the language context at the top level, making it available to all hooks. This centralization aligns with the Apollo cache optimization goals mentioned in the PR objectives.
Consider documenting this pattern in the codebase to ensure consistent language handling across other API modules.
34-34: Document the internal lang handlingConsider adding JSDoc documentation to clarify that language is now handled internally.
+/** + * Hook to query stories with internal language handling. + * @param props - Query properties excluding language which is handled internally + * @param options - Custom Apollo query options + */ const useStoriesQuery = useCallback(web/src/services/api/layersApi/index.ts (1)
Line range hint
37-47: Consider memoizing query options for performanceThe changes correctly implement the standardization of
langusage at the API level. However, the query options object is recreated on every render. Consider memoizing it for better performance:const useGetLayersQuery = useCallback( ({ sceneId }: LayerQueryProps) => { + const queryOptions = useMemo(() => ({ + variables: { sceneId: sceneId ?? "", lang }, + skip: !sceneId + }), [sceneId, lang]); + - const { data, ...rest } = useQuery(GET_SCENE, { - variables: { sceneId: sceneId ?? "", lang }, - skip: !sceneId - }); + const { data, ...rest } = useQuery(GET_SCENE, queryOptions); const nlsLayers = useMemo(() => getLayers(data), [data]); return { nlsLayers, ...rest }; }, [lang] );web/src/services/api/infoboxApi/blocks.ts (1)
58-58: LGTM: Improved architecture for language handlingMoving the language context to the top level using
useLanghook is a good architectural decision. This centralization:
- Reduces parameter passing
- Makes language handling consistent across all hooks
- Improves maintainability
web/src/services/api/storytellingApi/blocks.ts (2)
Line range hint
60-73: Consider optimizing the useMemo dependency.While the changes for language support are correct, the inner
useMemocould be optimized. Currently, it depends on the entiredataobject, which might cause unnecessary recalculations.Consider updating the dependency to only include the relevant part of the data:
const installableStoryBlocks = useMemo( () => getInstallableStoryBlocks(data), - [data] + [data?.node] );
Line range hint
74-97: Consider optimizing the useMemo dependency.Similar to the previous hook, while the language support changes are correct, the inner
useMemocould be optimized to prevent unnecessary recalculations.Consider updating the dependency to only include the relevant part of the data:
const installedStoryBlocks = useMemo( () => getInstalledStoryBlocks(data, storyId, pageId), - [data, storyId, pageId] + [data?.node, storyId, pageId] );web/src/services/api/widgetsApi/index.ts (2)
Line range hint
60-73: Consider memoizing the variables objectWhile the changes correctly implement language standardization, consider memoizing the variables object to prevent unnecessary re-renders:
({ sceneId }: WidgetQueryProps) => { + const variables = useMemo(() => ({ + sceneId: sceneId ?? "", + lang + }), [sceneId, lang]); + const { data, ...rest } = useQuery(GET_SCENE, { - variables: { sceneId: sceneId ?? "", lang }, + variables, skip: !sceneId });
Line range hint
77-87: Apply similar memoization pattern hereFor consistency with the previous hook and to optimize performance, consider applying the same variables memoization pattern here.
({ sceneId }: WidgetQueryProps) => { + const variables = useMemo(() => ({ + sceneId: sceneId ?? "", + lang + }), [sceneId, lang]); + const { data, ...rest } = useQuery(GET_SCENE, { - variables: { sceneId: sceneId ?? "", lang }, + variables, skip: !sceneId });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
web/src/beta/features/Editor/hooks/useScene.ts(1 hunks)web/src/services/api/infoboxApi/blocks.ts(3 hunks)web/src/services/api/layerStyleApi/index.ts(3 hunks)web/src/services/api/layersApi/index.ts(3 hunks)web/src/services/api/sceneApi.ts(2 hunks)web/src/services/api/storytellingApi/blocks.ts(4 hunks)web/src/services/api/storytellingApi/index.ts(3 hunks)web/src/services/api/widgetsApi/index.ts(4 hunks)
🔇 Additional comments (17)
web/src/beta/features/Editor/hooks/useScene.ts (2)
Line range hint 17-31: LGTM: Scene settings logic remains unchanged.
The core functionality for handling scene settings remains intact and properly implemented. The memoization and callback implementations are correct.
Line range hint 9-10: Verify language handling in useSceneQuery.
Based on the PR objectives to standardize lang usage at API hooks, we should verify that useSceneQuery internally handles the language context correctly. The removal of the explicit lang parameter suggests that language handling has been moved to the API layer.
Also applies to: 16-16
✅ Verification successful
UseSceneQuery Handles Language Context Correctly
The useSceneQuery function correctly manages the lang parameter internally as per the API layer standardization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that useSceneQuery internally handles lang parameter
# Check the implementation of useSceneQuery
ast-grep --pattern 'export const useSceneQuery = $$$'
# Check if useLang is used within the scene API implementation
rg -A 5 'useLang.*useSceneQuery'
# Check other scene query implementations for consistency
rg -A 5 'useScene.*Query'
Length of output: 4182
web/src/services/api/layerStyleApi/index.ts (2)
18-18: LGTM: Import added correctly
The useLang hook import is appropriately grouped with other i18n-related imports.
30-30: LGTM: Lang hook usage follows best practices
The useLang hook is correctly placed at the module level, making the language context available to all API functions.
web/src/services/api/storytellingApi/index.ts (2)
15-15: LGTM: Import addition aligns with standardization goals
The addition of useLang import alongside useT is consistent with the PR's objective to standardize language handling at the API level.
Line range hint 34-44: Verify impact on existing useStoriesQuery callers
The implementation correctly moves lang handling internally, but we should verify that all callers are updated accordingly.
web/src/services/api/layersApi/index.ts (3)
21-21: LGTM: Import addition aligns with standardization goal
The addition of useLang import alongside useT maintains good organization of i18n-related imports.
33-33: LGTM: Hook initialization follows established patterns
The useLang hook is properly initialized at the component level, ensuring consistent access to language context.
Line range hint 37-47: Verify Apollo cache behavior with language changes
Let's ensure that the Apollo cache correctly handles queries with different language parameters.
web/src/services/api/infoboxApi/blocks.ts (2)
22-22: LGTM: Import addition aligns with refactor goals
The addition of useLang import alongside useT is consistent with the PR's objective to standardize language handling at the API level.
Line range hint 61-74: Verify query behavior with language changes
The refactoring looks good:
- Simplified function signature
- Proper language inclusion in query variables
- Correct dependency tracking for language changes
However, let's verify that this change doesn't break existing query caching behavior.
✅ Verification successful
Query behavior with language changes verified successfully
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential issues with Apollo cache keys and language handling
# Look for other GraphQL queries that might need similar lang parameter handling
rg -l 'useQuery.*GET_SCENE' | while read -r file; do
echo "=== $file ==="
rg -A 5 'useQuery.*GET_SCENE' "$file"
done
# Check for any remaining direct lang parameter usage that might need refactoring
ast-grep --pattern 'function $_(lang: $_) {
$$$
}'
Length of output: 3224
web/src/services/api/storytellingApi/blocks.ts (3)
22-22: LGTM: Import addition aligns with internationalization needs.
The addition of useLang import alongside useT is appropriate for implementing consistent language handling.
57-57: LGTM: Proper hook initialization.
The useLang hook is correctly initialized at the component level, making the language context available for all child hooks.
Line range hint 60-97: Verify Apollo cache behavior with language parameter.
Let's verify that the Apollo cache is working effectively with the standardized language parameter.
web/src/services/api/widgetsApi/index.ts (3)
16-16: LGTM: Import addition aligns with standardization goals
The addition of useLang import alongside useT is consistent with the PR's objective to standardize language handling at the API level.
56-56: LGTM: Proper hook initialization
The useLang hook is correctly initialized at the top level, ensuring consistent language context across all API operations.
Line range hint 56-87: Verify Apollo cache effectiveness
The changes successfully standardize the lang parameter usage at the API level, which should improve Apollo cache utilization. To verify this improvement:
Overview
GET_SCENE query has prop
lang, but when we use it we sometimes passing in the value sometimes not, which will lead to we can't make full use of apollo cache (since the var is different with/without lang, cache could be updated again and again when there's request with/without lang trigger).BEFORE:
AFTER:
What I've done
Refactor apis always use lang directly at api level, it can prevent some unnecessary queries.
What I haven't done
How I tested
Which point I want you to review particularly
Memo