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.

Sign up for GitHub

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
Open

Deploy efficient questions monitoring #890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1fa691a
limit the number reads for questions monitoring
rgu0114 Nov 7, 2024
07012ac
remove console logs
rgu0114 Nov 7, 2024
0772b68
fix build issue
rgu0114 Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 35 additions & 6 deletions src/components/includes/SessionView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import React, { useState, useEffect, useCallback } from "react";
import { Icon } from "semantic-ui-react";

import { connect } from "react-redux";
import { useParams } from "react-router-dom";
import SessionInformationHeader from "./SessionInformationHeader";
import SessionQuestionsContainer from "./SessionQuestionsContainer";

Expand Down Expand Up @@ -52,10 +51,6 @@ type AbsentState = {
lastAskedQuestion: FireQuestion | null;
};

type RouteParams = {
courseId: string;
};

const SessionView = ({
course,
session,
Expand Down Expand Up @@ -87,7 +82,6 @@ const SessionView = ({
});

const sessionProfile = useSessionProfile(isTa ? user.userId : undefined, isTa ? session.sessionId : undefined);
const { courseId } = useParams<RouteParams>();

const updateSessionProfile = useCallback(
(virtualLocation: string) => {
Expand Down Expand Up @@ -122,6 +116,41 @@ const SessionView = ({
// setPrevQuestSet(new Set(questions.map(q => q.questionId)));
}, [questions, user.userId, course.courseId, user.roles, user, session.sessionId]);

/** This useEffect dictates when the TA feedback popup is displayed by monitoring the
* state of the current question. Firebase's [onSnapshot] method is used to monitor any
* changes to the questions collection, and [docChanges] filters this down to the document
* changes since the last snapshot. Then, we call [removeQuestionDisplayFeedback] iff a question was
* both modified and resolved, indicating that the TA has answered a question. !isTa and
* !isProf ensures that this useEffect only runs for students.
*/
// TODO (richardgu): use a Firebase Cloud Function for a server-side trigger in the future
useEffect(() => {
let unsubscribe: () => void;

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?

snapshot.docChanges().forEach((change) => {
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.

// eslint-disable-next-line no-console
removeQuestionDisplayFeedback(questionId);
}
});
});
}

return () => {
if (unsubscribe) {
unsubscribe();
}
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

const dismissUndo = () => {
if (timeoutId) {
clearTimeout(timeoutId);
Expand Down
6 changes: 1 addition & 5 deletions src/components/pages/SplitView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,21 +144,17 @@ const SplitView = ({
removeQuestionbyID(firestore, removeQuestionId);
};

// used when a student removes their own question, don't want to dispaly feedback
// used when a student removes their own question, don't want to display feedback
const setRemoveQuestionWrapper = (questionId: string | undefined) => {
setRemoveQuestionId(questionId);
setRemovedQuestionId(questionId);
// eslint-disable-next-line no-console
console.log("split view questionId: ", questionId);
};

// used to display feedback to user once question is removed
const removeQuestionDisplayFeedback = (questionId: string | undefined) => {
setRemoveQuestionId(questionId);
setDisplayFeedbackPrompt(true);
setRemovedQuestionId(questionId);
// eslint-disable-next-line no-console
console.log("split view questionId: ", questionId);
};

useEffect(() => {
Expand Down
Loading