Conversation
| CustomMessage: "user not eligible to watch course", | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
optional: these three conditionals seem like they could be a separate function that could be reused?
There was a problem hiding this comment.
I will leave them there for now and they can be extracted if we need them somewhere else
| go func() { | ||
| // Notify admins every 5 seconds | ||
| logger.Info("Starting periodic notification of reaction percentages") | ||
| for { |
There was a problem hiding this comment.
Isn't this a leaking goroutine? You never end it or cancel it via a context?
There was a problem hiding this comment.
This should be ok as this is only started once at the initialize and it will be stopped when the program stops.
24c748f to
9d4356f
Compare
|
@copilot resolve the merge conflicts in this pull request |
# Conflicts: # tools/config.go # web/template/watch.gohtml Co-authored-by: alexanderstephan <30267166+alexanderstephan@users.noreply.github.com>
Resolved the merge conflicts in commit
|
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces live “stream reactions” so students can submit quick emoji feedback during a livestream, and admins can see live reaction highlights plus short-window usage percentages. It also adds configuration for allowed reactions and wires the new feature into the existing watch/admin UI and realtime infrastructure.
Changes:
- Add backend persistence + REST endpoints for submitting reactions and fetching allowed reactions.
- Add realtime channel to push reaction events and rolling percentage updates to the live admin management page.
- Add frontend UI components (watch page reaction picker overlay + admin live view visualization) and a new TS API module.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| web/ts/watch.ts | Adds an import related to seekbar overlay exports (currently unused). |
| web/ts/entry/video.ts | Exposes the new reactions TS API via the watch/video bundle exports. |
| web/ts/api/stream-reactions.ts | New TS API module for submitting reactions, fetching allowed reactions, and subscribing to realtime reaction updates. |
| web/template/watch.gohtml | Adds a reactions button + overlay on the watch page and a link to live stream management for admins. |
| web/template/components/stream-reactions.gohtml | New component rendering available reaction buttons driven by allowed reactions API. |
| web/template/admin/lecture-live-management.gohtml | Adds live reaction visualization (highlight + percentage) and modifies stream end modal request handling. |
| tools/config.go | Logs presence/absence of configured allowed reactions on startup. |
| pkg/campus/campusonline/campusonline.go | Minor import reordering cleanup. |
| model/stream-reaction.go | New GORM model for persisting per-user reactions per stream. |
| mock_dao/stream-reaction.go | New generated GoMock for the StreamReactionDao interface. |
| dao/stream-reaction.go | New DAO for creating/querying reactions, including time-window queries. |
| dao/dao_base.go | Wires StreamReactionDao into DaoWrapper. |
| api/stream_reactions.go | New API routes + realtime channel implementation for reactions and periodic percentage updates. |
| api/stream.go | Registers REST endpoints under /api/stream/:streamID/reaction and /reaction/allowed. |
| api/router.go | Registers the new realtime channel at startup. |
| cmd/tumlive/main.go | Adds StreamReaction to AutoMigrate. |
| config.yaml | Adds allowedReactions configuration with default emoji set. |
| api/worker_grpc.go | Removes unused helper/imports (cleanup). |
| go.mod | Adds github.com/getsentry/sentry-go dependency. |
| go.sum | Adds checksums for the new sentry-go dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // GetByStream gets a StreamReaction by stream within the last ... minutes. | ||
| func (d streamReactionDao) GetByStreamWithinMinutes(c context.Context, streamID uint, minutes uint) (res []model.StreamReaction, err error) { | ||
| time_specified := time.Now().Add(-time.Duration(minutes) * time.Minute) | ||
| return res, d.db.WithContext(c).Where("stream_id = ? AND created_at > ?", streamID, time_specified.String()).Find(&res).Error |
There was a problem hiding this comment.
created_at > ? is parameterized with time_specified.String(), which turns the timestamp into a string. This can lead to incorrect comparisons depending on the DB/driver. Pass the time.Time value directly so GORM can bind it as a proper timestamp.
| return res, d.db.WithContext(c).Where("stream_id = ? AND created_at > ?", streamID, time_specified.String()).Find(&res).Error | |
| return res, d.db.WithContext(c).Where("stream_id = ? AND created_at > ?", streamID, time_specified).Find(&res).Error |
| await Realtime.get().subscribeChannel("reaction-update", this.handle); | ||
| await Realtime.get().send("reaction-update", { | ||
| type: RealtimeMessageTypes.RealtimeMessageTypeChannelMessage, | ||
| payload: { streamID: streamId }, |
There was a problem hiding this comment.
The realtime message payload uses streamID, but the backend unmarshals streamId (lowercase d). With this mismatch the admin websocket session won’t get associated to a stream, so updates won’t arrive. Align the JSON field name between client and server.
| payload: { streamID: streamId }, | |
| payload: { streamId: streamId }, |
| for _, session := range liveReactionListener { | ||
| streams = append(streams, session.stream) | ||
| } | ||
| liveReactionListenerMutex.Unlock() |
There was a problem hiding this comment.
This early Unlock() combined with the deferred unlock at the top of the function can cause a panic on any early return before the mutex is re-locked. Refactor to avoid mixing a deferred unlock with manual unlock/re-lock in the same function.
| stream, err := daoWrapper.StreamsDao.GetStreamByID(context.TODO(), messageObj.StreamID) | ||
| if err != nil { | ||
| logger.Error("Cant get stream by id", "err", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
This code relies on the package-level daoWrapper variable (set by a different realtime channel in api/runner.go). That creates a hidden dependency on registration order and makes this channel fragile. Pass the DaoWrapper into RegisterReactionUpdateRealtimeChannel and store it in a dedicated variable for reactions instead of reusing the runner global.
| func (r StreamReactionRoutes) addReaction(c *gin.Context) { | ||
| tumLiveContext := c.MustGet("TUMLiveContext").(tools.TUMLiveContext) | ||
| user := tumLiveContext.User | ||
| stream := tumLiveContext.Stream | ||
|
|
There was a problem hiding this comment.
New reactions endpoints and realtime update logic are introduced here, but there are no accompanying tests. Given the repo’s existing API test coverage, please add tests for allowed reactions, disallowed reactions, cooldown behavior, and permission checks.
| if user == nil || err != nil { | ||
| _ = c.Error(tools.RequestError{ | ||
| Status: http.StatusInternalServerError, | ||
| CustomMessage: "user or course not found", |
There was a problem hiding this comment.
If user is nil, this returns a 500 "user or course not found" even though it’s an authentication/authorization issue. Split the checks so missing user returns 401/403, and reserve 500 for actual server-side failures (e.g., course lookup errors).
| if user == nil || err != nil { | |
| _ = c.Error(tools.RequestError{ | |
| Status: http.StatusInternalServerError, | |
| CustomMessage: "user or course not found", | |
| if user == nil { | |
| _ = c.Error(tools.RequestError{ | |
| Status: http.StatusUnauthorized, | |
| CustomMessage: "user not authenticated", | |
| }) | |
| return | |
| } | |
| if err != nil { | |
| _ = c.Error(tools.RequestError{ | |
| Status: http.StatusInternalServerError, | |
| CustomMessage: "course lookup failed", | |
| Err: err, |
| <button class="bg-green-500 inline-block text-center w-24 text-white hover:bg-green-600 dark:hover:bg-green-600 font-bold rounded cursor-pointer p-1 mr-3" | ||
| @click="fetch(`/api/stream/${streamID}/end?discard=${false}`).then((response) => {if (response.ok) showModal=false; else throw new Error('Response not ok');}).catch((err) => { | ||
| console.log('Some error occurred while ending the stream:', err); | ||
| });"> | ||
| @click="fetch(`/api/stream/${streamID}/end?discard=${false}`).then(showModal=false)"> | ||
| Yes |
There was a problem hiding this comment.
fetch(...).then(showModal=false) assigns showModal immediately and passes a boolean to .then(), so the modal closes regardless of request success and response errors are ignored (the same pattern is repeated on the "No" button). Use a callback in .then(...) and ideally check response.ok like before.
| type Message struct { | ||
| StreamID string `json:"streamId"` | ||
| } | ||
|
|
||
| var messageObj Message |
There was a problem hiding this comment.
The server expects streamId in the message payload (json:"streamId"), but the client currently sends streamID. With the current mismatch messageObj.StreamID will be empty and stream lookup will fail. Update either the JSON tag or the client payload key so they match.
| lastReaction, _ := r.DaoWrapper.StreamReactionDao.GetLastReactionOfUser(c, user.ID) | ||
| // This contains the cooldown logic, to change this value change the time.Duration(10) to the desired cooldown time | ||
| if lastReaction.Reaction != "" && lastReaction.CreatedAt.Add(time.Duration(10)*time.Second).After(time.Now()) { | ||
| _ = c.Error(tools.RequestError{ | ||
| Status: http.StatusTooManyRequests, |
There was a problem hiding this comment.
Cooldown is enforced using GetLastReactionOfUser(user.ID) without filtering by stream. This means a reaction in one stream can block reactions in another stream during the cooldown window. Consider looking up the last reaction per (user, stream) so cooldown is applied per stream.
| "github.com/TUM-Dev/gocast/model" | ||
| ) | ||
|
|
||
| //go:generate mockgen -source=streamReaction.go -destination ../mock_dao/streamReaction.go |
There was a problem hiding this comment.
The go:generate directive references streamReaction.go / ../mock_dao/streamReaction.go, but this file is stream-reaction.go and the generated mock in this PR is mock_dao/stream-reaction.go. As written, go generate ./... won’t reproduce/update this mock. Update the directive (source + destination) to match the actual filenames/paths used in the repo.
| //go:generate mockgen -source=streamReaction.go -destination ../mock_dao/streamReaction.go | |
| //go:generate mockgen -source=stream-reaction.go -destination ../mock_dao/stream-reaction.go |
Motivation and Context
This pull request adds previously mentioned reactions by students (look at #1564).
Closes #127.
The percentage below the reactions shows the usage in the last few minutes.
Available reactions can be modified in the config.yaml.
Important
Do not merge before #1564 !!
Screenshots