enhance(apps/frontend-manage): make sure that live quiz deletion workflow is consistent with other deletions #4433
Conversation
…flow is consistent with other deletions
Current Aviator status
This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
|
Warning Rate limit exceeded@sjschlapbach has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 26 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive refactoring of live quiz deletion functionality across multiple files. The primary change involves replacing the generic Changes
Possibly related PRs
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
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 (6)
apps/frontend-manage/src/components/courses/modals/LiveQuizDeletionModal.tsx (1)
8-14: Well-patterned interface definition.
TheLiveQuizDeletionModalPropsinterface succinctly lists required props and data types. Consider adding inline documentation if the modal’s usage expands.apps/frontend-manage/src/components/liveQuiz/LiveQuiz.tsx (2)
83-106: Validate potential error states for thedeleteLiveQuizmutation.While the mutation handles optimistic updates and cache writes, it would be prudent to handle error states more explicitly (e.g., showing an error toast if the deletion fails).
383-388: Ensure consistent naming of modal props.The props
openandsetOpenare clear, but watch for consistency. In other parts of the code, similar toggles might appear asisOpenoronClose.packages/graphql/src/schema/query.ts (1)
38-38: Remove unused exports if applicable.The addition of
LiveQuizSummaryaligns with the removedRunningLiveQuizSummary. IfRunningLiveQuizSummaryis no longer in use anywhere, ensure it is fully removed to reduce clutter.Do you want me to open a new GitHub issue for scanning and removing any residual references to
RunningLiveQuizSummaryacross the codebase?packages/graphql/src/ops.ts (1)
1034-1040: Consider adding a descriptive comment forLiveQuizSummary.
Providing a brief JSDoc or comment for each field clarifies the meaning of statistics (e.g.,numOfConfusionFeedbacks,numOfFeedbacks, etc.) and helps future maintainers understand how these numbers are generated and used.packages/graphql/src/ops.schema.json (1)
10621-10624: Add descriptions to improve schema documentation.Consider adding meaningful descriptions to the
LiveQuizSummarytype and its fields to improve schema documentation and API clarity.{ "kind": "OBJECT", "name": "LiveQuizSummary", - "description": null, + "description": "Summary of a live quiz including participation and feedback metrics", "isOneOf": null,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/frontend-manage/src/components/courses/LiveQuizElement.tsx(3 hunks)apps/frontend-manage/src/components/courses/modals/LiveQuizDeletionModal.tsx(1 hunks)apps/frontend-manage/src/components/liveQuiz/LiveQuiz.tsx(3 hunks)apps/frontend-manage/src/components/liveQuiz/cockpit/LiveQuizAbortionConfirmations.tsx(1 hunks)packages/graphql/src/ops.schema.json(2 hunks)packages/graphql/src/ops.ts(3 hunks)packages/graphql/src/public/schema.graphql(2 hunks)packages/graphql/src/schema/liveQuiz.ts(1 hunks)packages/graphql/src/schema/query.ts(2 hunks)packages/i18n/messages/de.ts(1 hunks)packages/i18n/messages/en.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: format
- GitHub Check: test
- GitHub Check: build
- GitHub Check: Analyze (javascript)
- GitHub Check: cypress-run
- GitHub Check: SonarCloud
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (25)
apps/frontend-manage/src/components/liveQuiz/cockpit/LiveQuizAbortionConfirmations.tsx (2)
1-1: Import alignments look good.
This updated import fromLiveQuizSummarycorrectly reflects the renamed GraphQL type, keeping the code aligned with the new schema.
9-9: Prop type updated to match new schema.
The switch tosummary: LiveQuizSummaryaccurately aligns this component’s props with the newerLiveQuizSummarytype.apps/frontend-manage/src/components/courses/modals/LiveQuizDeletionModal.tsx (8)
1-6: Imports are consistent and organized.
All necessary modules for the new modal are being imported correctly.
16-22: Functional component signature is clear.
Use of destructured props is consistent with best practices and ensures readability.
23-32: Efficient GraphQL query usage.
Skipping the query when the modal is closed prevents unnecessary requests. Good job on controlling the fetch withskip: !open.
33-39: Local state initialization is straightforward.
Storing multiple confirmation flags underconfirmationskeeps logic tidier than having separate states for each. No issues noted.
40-46: Conditional refetch on open is well-implemented.
The effect hook withopenas a dependency prevents extraneous fetches when the modal is closed. This approach is optimal for a frequently toggled modal.
47-58: Automatic confirmations setup.
Initializing the confirmations using the fetched summary keeps user selections relevant. Be mindful if any item’s count changes while the modal remains open. You may need to re-handle partial refreshes or new data.
60-75: Graceful handling of absent data.
Returningnullif the summary is missing avoids potential undefined access errors. Implementation is safe.
76-134: Confirmation items & main content structure.
The multipleConfirmationItemcomponents are well-managed, consistently referencing the summary fields. The logic for enabling/disabling confirmations is clear.packages/graphql/src/schema/liveQuiz.ts (2)
205-205: Interface rename is consistent.
Renaming toILiveQuizSummaryhelps unify naming. The properties remain relevant to overall deletion logic.
211-213: Builder object ref alignment.
Renaming the refs toLiveQuizSummaryRefandLiveQuizSummaryis consistent with the new type name. All fields appear correct.apps/frontend-manage/src/components/courses/LiveQuizElement.tsx (3)
34-34: Nicely switching to the new modal import.
Replacing the oldDeletionModalwithLiveQuizDeletionModalreflects the updated approach to live quiz deletions.
90-122: Mutation usage for quiz deletion is well-structured.
Your approach includes:
- Clear variable passing (
variables: { id: quiz.id })- Cache updates removing the deleted quiz
- An optimistic response to keep the UI in sync
- Refetching the
GetSingleCourseDocumentto stay currentThis flow is coherent and consistent with best practices using Apollo Client.
291-296: Integration of the newLiveQuizDeletionModalis correct.
Passing the quiz ID, open state, and the delete function is succinct. ThedeletingLiveQuizloading state ensures proper feedback to the user.apps/frontend-manage/src/components/liveQuiz/LiveQuiz.tsx (1)
30-30: Good adoption of a specialized modal component.Replacing the generic deletion modal with a dedicated
LiveQuizDeletionModalis a clean approach that improves clarity and maintainability.packages/graphql/src/schema/query.ts (1)
262-262: Confirm backward-compatibility of field type changes.Changing
getLiveQuizSummaryto returnLiveQuizSummarymay affect older clients expectingRunningLiveQuizSummary. Verify that all references and front-end queries have been updated to the new type.packages/i18n/messages/en.ts (1)
1346-1356: Translations clarify deletion implications.Updating these strings to emphasize that participant scores remain unchanged is helpful for user transparency. Ensure that all mention of “results” or “feedbacks” is consistent across the UI.
packages/i18n/messages/de.ts (1)
1358-1367: Well-aligned German translations for the new deletion flow.The messages now clearly convey that the quiz data is irreversibly deleted while points remain unaffected. This consistency increases user trust.
packages/graphql/src/ops.ts (2)
2193-2193: Verify backward compatibility forgetLiveQuizSummary.
Ensure that any client code referencing the old return type (RunningLiveQuizSummary) is updated and that no references to the removed type remain.
3895-3895: Double-check queries to avoid breaking changes.
Ensure the front-end query or any other consumers are updated to handle the new fields and the changed type as intended. Consider verifying it in integration tests to confirm correct usage of the new summary model.
[approve]packages/graphql/src/public/schema.graphql (2)
948-953: LGTM! The newLiveQuizSummarytype is well-structured.The type definition follows GraphQL schema conventions with clear, descriptive field names and appropriate non-nullable integer types for the metrics.
1415-1415: Verify the impact of return type change.The query's return type has been updated from
RunningLiveQuizSummarytoLiveQuizSummary. This change aligns with the PR objective of making the quiz deletion workflow more consistent.Let's verify that all GraphQL queries have been updated to handle the new return type:
✅ Verification successful
Return type change has been properly implemented
The change from
RunningLiveQuizSummarytoLiveQuizSummaryhas been consistently implemented across the server and client code. All components accessing this query are using the correct fields from the new type.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to RunningLiveQuizSummary # and verify that LiveQuizSummary is used consistently. # Test 1: Search for any remaining references to RunningLiveQuizSummary echo "Checking for remaining references to RunningLiveQuizSummary..." rg "RunningLiveQuizSummary" # Test 2: Verify the usage of LiveQuizSummary in GraphQL queries echo "Checking LiveQuizSummary usage in queries..." rg "getLiveQuizSummary.*LiveQuizSummary"Length of output: 1863
Script:
#!/bin/bash # Check for client-side usage and test files echo "Checking for client-side GraphQL queries..." rg -t ts -t tsx "getLiveQuizSummary.*query" --glob '!*.test.*' --glob '!**/ops.ts' echo "Checking test files for any references..." rg -t ts -t tsx "getLiveQuizSummary" --glob '*.test.*'Length of output: 385
Script:
#!/bin/bash # Search for client-side usage with broader patterns echo "Checking for client-side GraphQL query usage..." rg "getLiveQuizSummary.*query" --glob '!*.test.*' --glob '!**/ops.ts' --glob '!**/schema.graphql' echo "Checking for any getLiveQuizSummary references in test files..." rg "getLiveQuizSummary" --glob '*.test.*' echo "Checking for any getLiveQuizSummary usages in client code..." rg "getLiveQuizSummary" --glob '!*.test.*' --glob '!**/ops.ts' --glob '!**/schema.graphql'Length of output: 3500
packages/graphql/src/ops.schema.json (2)
10625-10640: LGTM: New feedback fields enhance deletion workflow.The addition of
numOfConfusionFeedbacksandnumOfFeedbacksfields will help users make more informed decisions during quiz deletion by showing the amount of feedback data that would be lost.Also applies to: 10641-10656
10619-10694: Verify the impact of renaming RunningLiveQuizSummary.The type renaming from
RunningLiveQuizSummarytoLiveQuizSummaryis a breaking change that might affect API clients. Please ensure all consumers are updated accordingly.Let's verify the impact:
Also applies to: 20880-20880
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
live-quiz-deletion-modal
|
| Run status |
|
| Run duration | 13m 17s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
173
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/i18n/messages/en.ts (1)
1345-1355: Improve punctuation consistency in deletion messages.The messages effectively communicate the deletion workflow. Consider standardizing the punctuation:
- noFeedbacksToDelete: - 'For this live quiz no questions have been submitted in the Q&A channel yet.', + noFeedbacksToDelete: + 'For this live quiz no questions have been submitted in the Q&A channel yet', - noConfusionFeedbacksToDelete: - 'For this live quiz no confusion feedbacks have been submitted yet.', + noConfusionFeedbacksToDelete: + 'For this live quiz no confusion feedbacks have been submitted yet',apps/frontend-manage/src/components/courses/modals/TagDeletionModal.tsx (1)
20-26: Consider adding explicit error handling
IfdeleteTagfails, you may want to incorporate anonErrorcallback in theuseMutationoptions to provide user feedback or log the error for diagnostic purposes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/frontend-manage/src/components/courses/modals/DeletionModal.tsx(0 hunks)apps/frontend-manage/src/components/courses/modals/TagDeletionModal.tsx(1 hunks)apps/frontend-manage/src/components/questions/tags/TagActions.tsx(2 hunks)packages/i18n/messages/de.ts(2 hunks)packages/i18n/messages/en.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/frontend-manage/src/components/courses/modals/DeletionModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/i18n/messages/de.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Analyze (javascript)
- GitHub Check: check
- GitHub Check: SonarCloud
- GitHub Check: cypress-run
- GitHub Check: build
🔇 Additional comments (10)
packages/i18n/messages/en.ts (1)
947-948: LGTM! Clear and informative deletion confirmation message.The message effectively communicates:
- What will be deleted (tag name in bold)
- What remains intact (questions with the tag)
- The irreversible nature of the action
apps/frontend-manage/src/components/courses/modals/TagDeletionModal.tsx (6)
1-6: Imports are properly structured
Looks good—each required dependency is clearly imported.
11-16: Prop definitions look consistent
The props defined here accurately describe the data needed for this modal: an ID, a name, and visibility state. This is a straightforward interface that clearly communicates the component's contract.
27-41: Cache update is done correctly
The logic that modifies the user tags array after deletion is accurate. It filters out the deleted tag ID from the cache—no issues here.
42-49: Great use of refetchQueries and optimisticResponse
Refetching questions ensures a consistent UI. The optimistic response helps maintain great responsiveness.
51-91: Well-structured modal UI
Using a dedicated modal component with clear confirm/cancel handling simplifies user interactions. The usage oftwMergefor styling is consistent. Consider verifying accessibility attributes (e.g., ensuring keyboard focus remains within the dialog) if not already handled globally.
93-93: Export default
Neat: a self-contained component is properly exported for reuse.apps/frontend-manage/src/components/questions/tags/TagActions.tsx (3)
8-8: Type import
Importing theTaginterface ensures type-safety when implementing the UI logic based on the tag data.
11-11: Explicit import
Good move: usingTagDeletionModalfrom a dedicated file fosters better separation of concerns and keeps the component code lean.
89-91: Conditional modal rendering appears correct
RenderingTagDeletionModalonly when needed helps avoid unnecessary overhead. Theidandnameprops ensure a consistent, context-aware deletion flow.
|
sjschlapbach
changed the title
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
v3
|
| Run status |
|
| Run duration | 13m 19s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
0
|
|
Flaky
|
0
|
|
Pending
|
0
|
|
Skipped
|
0
|
|
Passing
|
173
|
| View all changes introduced in this branch ↗︎ | |
Summary by CodeRabbit
New Features
LiveQuizDeletionModalfor managing live quiz deletions.TagDeletionModalfor tag deletion confirmations.Bug Fixes
Documentation
Refactor
RunningLiveQuizSummarytoLiveQuizSummaryacross multiple files.DeletionModal.