fix: ensure that live quiz abortion modal also takes into account cached responses #4462
Conversation
…y triggered when the modal is open
📝 WalkthroughWalkthroughThe pull request encompasses changes across multiple frontend components and a GraphQL service, focusing on optimizing data fetching and query execution in various modals related to courses, quizzes, and participant interactions. The modifications primarily involve removing manual query refetching, adding conditional query skipping based on modal open states, and enhancing response calculation for live quizzes. These changes aim to improve performance and reduce unnecessary API calls by executing queries only when necessary. Changes
Possibly related PRs
Suggested reviewers
Finishing Touches
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
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
apps/frontend-manage/src/components/questions/manipulation/ElementEditModal.tsx (1)
84-84: LGTM! Excellent optimization of the query execution.The addition of
|| !isOpento the skip condition is a valuable optimization that prevents unnecessary API calls when the modal is closed, while ensuring data is available when needed.This change aligns well with GraphQL best practices by:
- Reducing unnecessary network requests
- Loading data only when it's actually needed for rendering
- Maintaining consistency with similar optimizations across other modal components
apps/frontend-manage/src/components/liveQuiz/cockpit/LiveQuizQRModal.tsx (1)
Line range hint
16-18: Consider handling cache misses.The
cache-onlyfetch policy is good for performance, but consider handling cases where the user profile data might not be in the cache.const { data } = useQuery(UserProfileDocument, { fetchPolicy: 'cache-only', }) + +// Handle potential cache miss +if (!data?.userProfile) { + return ( + <Button disabled className={{ root: '!mr-0 w-[41%] sm:w-max' }}> + <Button.Icon> + <FontAwesomeIcon icon={faQrcode} /> + </Button.Icon> + {t('manage.general.qrCode')} + </Button> + ) +}apps/frontend-manage/src/components/courses/modals/MicroLearningDeletionModal.tsx (2)
Line range hint
34-47: Consider enhancing the optimistic response.The optimistic response could include more fields to better match the actual mutation response and prevent UI flickers.
optimisticResponse: { __typename: 'Mutation', deleteMicroLearning: { __typename: 'MicroLearning', id: activityId, + name: summaryData?.getMicroLearningSummary?.name, + status: 'DELETED' }, },
Line range hint
53-63: Consider using more specific effect dependencies.Instead of watching the entire summary object, consider watching only the specific fields needed for the effect.
- }, [summaryData?.getMicroLearningSummary]) + }, [ + summaryData?.getMicroLearningSummary?.numOfResponses, + summaryData?.getMicroLearningSummary?.numOfAnonymousResponses + ])apps/frontend-manage/src/components/liveQuiz/EmbeddingModal.tsx (1)
112-112: Consider applying skipQuery consistently across all LazyHMACLink instances.While the optimization is correctly implemented here, there are two other instances of
LazyHMACLinkin the file (within the elements map and for the leaderboard) that could benefit from the same optimization.Apply this pattern to other instances:
<LazyHMACLink quizId={quizId} params={`questionIx=${ix}&hideControls=true&showSolution=${showSolution}`} identifier={`question-${ix}`} + skipQuery={!open} /> <LazyHMACLink quizId={quizId} params={`leaderboard=true&hideControls=true`} identifier={`leaderboard`} + skipQuery={!open} />apps/frontend-pwa/src/components/participant/ParticipantProfileModal.tsx (1)
Line range hint
34-46: Consider performance and type safety improvements.While the implementation is solid, consider these enhancements:
- Memoize navigation functions using
useCallbackto prevent recreating them on each render- Add type safety for array index operations
Here's how you could implement these improvements:
+ import { useCallback } from 'react' - const onNext = () => { + const onNext = useCallback(() => { const nextIndex = (currentIndex + 1) % top10Participants.length setSelectedParticipant(top10Participants[nextIndex]) setCurrentIndex(nextIndex) - } + }, [currentIndex, top10Participants]) - const onPrev = () => { + const onPrev = useCallback(() => { const prevIndex = (currentIndex - 1 + top10Participants.length) % top10Participants.length setSelectedParticipant(top10Participants[prevIndex]) setCurrentIndex(prevIndex) - } + }, [currentIndex, top10Participants]) // Add type guard for array access + const getParticipantAtIndex = (index: number): string => { + if (index >= 0 && index < top10Participants.length) { + return top10Participants[index] + } + return top10Participants[0] + }apps/frontend-manage/src/components/courses/modals/GroupActivityDeletionModal.tsx (1)
26-32: Improved query optimization with conditional execution!The addition of
skip: !openis a good optimization that prevents unnecessary API calls when the modal is closed. This change aligns well with Apollo Client's caching strategy and reduces server load.Consider implementing a similar pattern in other modals that fetch data, as this approach:
- Prevents unnecessary network requests
- Leverages Apollo's cache effectively
- Reduces server load
- Simplifies the component's lifecycle management
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/frontend-manage/src/components/courses/modals/GroupActivityDeletionModal.tsx(1 hunks)apps/frontend-manage/src/components/courses/modals/GroupActivityEndingModal.tsx(1 hunks)apps/frontend-manage/src/components/courses/modals/LiveQuizDeletionModal.tsx(1 hunks)apps/frontend-manage/src/components/courses/modals/MicroLearningDeletionModal.tsx(1 hunks)apps/frontend-manage/src/components/courses/modals/MicroLearningEndingModal.tsx(1 hunks)apps/frontend-manage/src/components/courses/modals/PracticeQuizDeletionModal.tsx(1 hunks)apps/frontend-manage/src/components/liveQuiz/EmbeddingModal.tsx(2 hunks)apps/frontend-manage/src/components/liveQuiz/cockpit/CancelLiveQuizModal.tsx(1 hunks)apps/frontend-manage/src/components/liveQuiz/cockpit/LiveQuizQRModal.tsx(1 hunks)apps/frontend-manage/src/components/questions/manipulation/ElementEditModal.tsx(1 hunks)apps/frontend-pwa/src/components/participant/ParticipantProfileModal.tsx(1 hunks)packages/graphql/src/services/liveQuizzes.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run
🔇 Additional comments (15)
apps/frontend-manage/src/components/courses/modals/MicroLearningEndingModal.tsx (2)
26-32: LGTM! Improved query execution control.The changes effectively optimize data fetching by:
- Using Apollo's
skipoption to control query execution based on modal state- Removing unnecessary manual refetching, letting Apollo's cache handle data consistency
This approach is more efficient as it:
- Prevents unnecessary network requests when the modal is closed
- Leverages Apollo's built-in caching capabilities
Line range hint
34-71: Appreciate the thorough cache update implementation.The mutation's optimistic response and cache update logic are well implemented, ensuring a smooth user experience by:
- Immediately reflecting the state change in the UI
- Properly updating the course data in Apollo's cache
apps/frontend-manage/src/components/courses/modals/LiveQuizDeletionModal.tsx (1)
24-30: Great optimization of the query execution!The addition of the
skipoption and removal of manual refetching is a good improvement that:
- Prevents unnecessary API calls when the modal is closed
- Simplifies the code by leveraging Apollo Client's built-in caching
- Maintains consistency with similar optimizations in other modal components
apps/frontend-manage/src/components/liveQuiz/cockpit/LiveQuizQRModal.tsx (2)
47-47: LGTM! Good internationalization practice.The change to use the translation key improves the component's internationalization support.
Line range hint
16-18: Good use of cache-only fetch policy.Using
cache-onlyfetch policy is optimal here as it prevents unnecessary network requests, assuming the user profile is already in the cache from a parent component.apps/frontend-manage/src/components/liveQuiz/cockpit/CancelLiveQuizModal.tsx (1)
48-51: Great optimization!The addition of the
skipoption and removal of manual refetching is a good optimization. The query will now only execute when the modal is open, preventing unnecessary API calls.packages/graphql/src/services/liveQuizzes.ts (2)
1615-1619: Good addition of activeBlock inclusion.Including the activeBlock and its elements in the query is necessary to handle cached responses, which aligns with the PR objective.
1625-1653: Well-structured response calculation enhancement.The response calculation has been improved to handle both completed and active blocks:
- Calculates responses from completed blocks
- Adds responses from the active block using cached results
The implementation is robust with proper null checks and safe accumulation of responses.
apps/frontend-manage/src/components/courses/modals/MicroLearningDeletionModal.tsx (1)
26-32: Great optimization of the query execution!The addition of
skip: !openand removal of manual refetching is a good improvement that:
- Prevents unnecessary API calls when the modal is closed
- Leverages Apollo Client's built-in caching more effectively
- Reduces component complexity by removing manual query management
apps/frontend-manage/src/components/liveQuiz/EmbeddingModal.tsx (2)
17-17: LGTM! Well-structured prop addition.The new
skipQueryparameter is properly typed and includes a sensible default value, maintaining backward compatibility.Also applies to: 22-22
28-28: Great performance optimization!Using Apollo's
skipoption prevents unnecessary network requests when the modal is closed, reducing server load and improving application performance.apps/frontend-manage/src/components/courses/modals/PracticeQuizDeletionModal.tsx (1)
26-32: LGTM! Optimization looks good.The addition of
skip: !openparameter optimizes data fetching by preventing unnecessary API calls when the modal is closed. The removal of manual refetching also simplifies the component's logic.Let's verify that this pattern is consistently applied across other similar modals:
✅ Verification successful
Optimization pattern verified across all modal components ✅
The
skip: !openoptimization is consistently implemented across all 7 modal components, and no manual refetch patterns were found in the codebase. This confirms that the optimization has been properly standardized.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if other modal components follow the same pattern # Expected: Similar useQuery implementations with skip logic # Search for modal components with useQuery ast-grep --pattern 'useQuery($_, { $$$ skip: !open, $$$ })' # Verify there are no remaining manual refetch patterns rg -l "refetch.*useEffect.*open" "apps/frontend-manage/src/components/**/modals/*.tsx"Length of output: 5240
Script:
#!/bin/bash # Fix the path pattern for rg command to properly search modal components rg -l "refetch.*useEffect.*open" "apps/frontend-manage/src/components"Length of output: 70
apps/frontend-pwa/src/components/participant/ParticipantProfileModal.tsx (1)
29-29: LGTM! Optimization prevents unnecessary API calls.The addition of
skip: !isProfileModalOpenis a good optimization that prevents unnecessary GraphQL queries when the modal is closed.Let's verify the cache configuration to ensure data consistency:
✅ Verification successful
Verified: Skip optimization is safe with Apollo's caching
The skip optimization is well-implemented as:
- Apollo's default cache normalization handles the participant profile data structure appropriately
- Re-fetching on modal open ensures fresh data when needed
- Profile data is relatively static between views
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Apollo cache configuration and related components # Look for Apollo cache configuration rg -A 10 'new ApolloClient|InMemoryCache' # Check for cache-related policies in type policies ast-grep --pattern 'new InMemoryCache({ $$$ typePolicies: { $$$ } $$$ })' # Look for related participant profile queries to verify cache behavior rg 'GetPublicParticipantProfile'Length of output: 13514
apps/frontend-manage/src/components/courses/modals/GroupActivityDeletionModal.tsx (1)
26-32: Verify the removal of useEffect and refetch.Based on the AI summary, the manual refetch logic was removed. This is a good change as it eliminates redundant data fetching, but we should verify that no edge cases are introduced.
Let's check for any remaining manual refetch patterns in other modal components:
✅ Verification successful
The removal of useEffect and refetch is safe and correct.
The removed pattern was specific to redundant data fetching, while other modals use useEffect and refetchQueries appropriately for UI state management and data consistency. No similar patterns requiring optimization were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential useEffect + refetch patterns in other modal components # that might need similar optimization # Look for modal components with useEffect and refetch rg -l 'useEffect.*refetch' apps/frontend-manage/src/components/courses/modals/Length of output: 80
Script:
#!/bin/bash # First, let's check the specific file's history git log -p apps/frontend-manage/src/components/courses/modals/GroupActivityDeletionModal.tsx | grep -A 10 "useEffect" # Then, let's search for useEffect and refetch patterns more broadly echo "=== Searching for useEffect in modal components ===" rg "useEffect" apps/frontend-manage/src/components/courses/modals/ -A 5 echo "=== Searching for refetch patterns in modal components ===" rg "refetch|useLazyQuery" apps/frontend-manage/src/components/courses/modals/ -A 5Length of output: 18305
apps/frontend-manage/src/components/courses/modals/GroupActivityEndingModal.tsx (1)
27-33: Great optimization of the data fetching strategy!The addition of
skip: !openis a good improvement that prevents unnecessary API calls when the modal is closed. The removal of manual refetching also simplifies the component's logic while maintaining functionality through Apollo's cache.Let's verify that the cached data remains fresh for the user experience:
✅ Verification successful
Optimization approved: The data fetching strategy is safe and efficient
The changes are well-implemented because:
- The
skip: !openflag correctly prevents unnecessary API calls when the modal is closed- Data freshness is maintained through Apollo's cache and real-time subscription updates (SGroupActivityStarted, SGroupActivityEnded)
- Removal of manual refetching simplifies the code while maintaining data consistency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are other components that might update the group activity summary data # and whether the cache is properly invalidated in those cases. # Look for mutations that might affect group activity summary ast-grep --pattern 'mutation { $$$ groupActivity$$$ $$$ }' # Look for cache updates in related components rg -l "GetGroupActivitySummaryDocument" | xargs rg "cache\.(modify|writeQuery)"Length of output: 146
Script:
#!/bin/bash # Search for GraphQL files with group activity operations echo "=== GraphQL Operations ===" fd -e graphql -e gql | xargs rg -l "groupActivity" # Look for mutation components and hooks echo -e "\n=== Mutation Usage ===" rg -l "useMutation.*groupActivity" # Check for cache configurations echo -e "\n=== Cache Configuration ===" fd . -e ts -e tsx | xargs rg "cache: {|cacheConfig|typePolicies"Length of output: 1089
Script:
#!/bin/bash # Look for Apollo client setup and cache configuration echo "=== Apollo Configuration ===" rg "new ApolloClient|cache:" # Look for subscription handlers echo -e "\n=== Subscription Handlers ===" rg -A 5 "useSubscription.*GroupActivity" # Search for cache updates with proper pattern echo -e "\n=== Cache Updates ===" rg "cache\s*:\s*{|writeQuery|modify"Length of output: 67609
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
optimized-modal-loading
|
| Run status |
|
| Run duration | 16m 37s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
254
|
| View all changes introduced in this branch ↗︎ | |
Summary by CodeRabbit
Performance Optimization
Localization
Query Handling
getLiveQuizSummaryfunction to include responses from active and completed quiz blocksskipQueryoption to conditionally execute GraphQL queries in modals