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

Deploy efficient questions monitoring #890

Open
wants to merge 3 commits into
base: release
Choose a base branch
from

Conversation

rgu0114
Copy link
Contributor

@rgu0114 rgu0114 commented Nov 8, 2024

Summary

Deploy efficient questions monitoring and a follow-up pr to fix some build issues.

Test Plan

Notes

Breaking Changes

None

  • I have updated the documentation accordingly.
  • My PR adds a @ts-ignore

@rgu0114 rgu0114 requested a review from a team as a code owner November 8, 2024 22:44
@dti-github-bot
Copy link
Member

[diff-counting] Significant lines: 41.

const questionData = change.doc.data();
const questionId = change.doc.id;

if (change.type === "modified" && questionData.status === "resolved") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is only removing the questions whose status are resolved and and has been modified, would it be better to first filter the array of docChanges to keep only those that are modified and whose status is resolved. This may be optimal in cases when docChanges is big, thus we can avoid iterating through each change.

if (!isTa && !isProf) {
const questionsRef = firestore.collection("questions").where("sessionId", "==", session.sessionId);

unsubscribe = questionsRef.onSnapshot((snapshot) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Richard! I really like your detailed comment on what the useEffect is, it was very helpful in understanding the useEffect! Overall it looks good to me, but I was wondering if it would be a good idea to handle any potential listening errors for onSnapshot?

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.

4 participants