Skip to content

Sessions Overhaul #3140

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

Jump to bottom
Open
wants to merge 53 commits into
base: master
Choose a base branch
from
Open

Sessions Overhaul #3140

wants to merge 53 commits into from
+994 −440

Conversation

TheMythologist
Copy link
Contributor

@TheMythologist TheMythologist commented Jun 23, 2025 •
edited
Loading

Description

CP3108 Project by @salmonkarp and myself
Fixes sessions, as well as better session integration with changes to sessions backend

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Code quality improvements

Linked to source-academy/sharedb-ace#30 and source-academy/sharedb-ace-backend#33

salmonkarp and others added 30 commits February 6, 2025 05:46
@salmonkarp
Feature: Add temporary session ID indicator in primary popup and plac…
2afd465 
…eholder Session Management tab in Playground
@salmonkarp
Merge remote-tracking branch 'origin/master' into sessions-overhaul
60a4ae8
@TheMythologist
Add AceMultiSelectionManager"
4324746
@TheMythologist
Revert first commit
c57c738
@TheMythologist
Add AceRadarView
2548634
@salmonkarp
Add basic information to Session Management Tab
862c17a
@TheMythologist
Clean up useShareAce implementation
727284f
@TheMythologist
Merge branch 'sessions-overhaul' of https://github.com/source-academy…
5ffaa89 
…/frontend into sessions-overhaul
@TheMythologist
Use proper role enum
0862623
@salmonkarp
Basid user display with updated binding
fbf3de7
@TheMythologist
Cleanup and fix missing ace-build require
bec06c5
@TheMythologist
Implement changing of language
846571e
@TheMythologist
More cleanup and fix invalid source chapter strings
696b7cc
@TheMythologist
Cleanup WIP
0bc2bf1
@TheMythologist
Cleanup Part 2
d2c5ebb
@TheMythologist
Implement changing of roles
1cea88e
@TheMythologist
Combine viewing and editing sessionIds into 1
fc9fa73
@TheMythologist
Refactor to functional component to prepare for local storage
bfab102
@TheMythologist
Update moduleResolution to bundler
6c8dd11
@salmonkarp
Add description and styles of management tab
cfbd599
@salmonkarp
Simplify session creation/joining/leaving into one popover
7b42fa3
@salmonkarp
Cleanup and add toggle of default role upon join
a0efc24
@salmonkarp
Add prototype link sharing
fb535d1
@salmonkarp
Add bad navigation bar fix
1fcbd8e
@salmonkarp
Add consistent URL behaviour on session creation and exit
e07521b
@TheMythologist
ui: hide toggle for admins
e4f0119
@TheMythologist
ui: Reflect ownership in button modal
1aeb5b1
@TheMythologist
cleanup
9867f68
@TheMythologist
UI cleanup
679bfdd
@TheMythologist
hotfix
200d5e9
RichDom2185
RichDom2185 reviewed Jun 25, 2025
View reviewed changes
Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

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

image

I'm still getting some type errors after linking locally and running yarn build?

src/commons/navigationBar/NavigationBar.tsx Outdated
!Constants.playgroundOnly &&
isEnrolledInACourse &&
!isMobileBreakpoint &&
!window.location.href.includes('/playground/') ? ( //TODO: this is bad
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain:

  1. The rationale of adding this condition
  2. What a better alternative might be

Instead of just "this is bad" please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@salmonkarp

Copy link

@salmonkarp salmonkarp Jun 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Think this is a hotfix to disable that navigation bar when I directly paste a URL generated from a session into the address bar. Normally going to the Playground would disable it, but doing so from the generated link didn't. Looking into it.

Copy link

Choose a reason for hiding this comment

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

Changed in recent commit

@TheMythologist
Copy link
Contributor Author

Hi @RichDom2185 I've fixed the errors, using the solution provided here: reduxjs/redux-toolkit#4900 (comment)

@TheMythologist
Copy link
Contributor Author

Also, do let me know when the npm package @sourceacademy/sharedb-ace is updated so I can bump the version here

@TheMythologist
Copy link
Contributor Author

Are there any issues with bumping the @sourceacademy/sharedb-ace npm package? So I can update this PR 👍

Or if there's any other comments to address, do let me know, so we can speed up the addition of this feature!

@RichDom2185
Copy link
Member

Are there any issues with bumping the @sourceacademy/sharedb-ace npm package? So I can update this PR 👍

Or if there's any other comments to address, do let me know, so we can speed up the addition of this feature!

Sorry, forgot to update. I don't have access to bump it on NPM. Prof @martin-henz , could you bump it to v2.1.0 as specified in the package.json? No need to increment to 2.1.1, it's already incremented from the PR, just need to publish.

@TheMythologist
Copy link
Contributor Author

Hi, bumping this @RichDom2185 and prof @martin-henz

@martin-henz
Copy link
Member

OK, just published: https://www.npmjs.com/package/@sourceacademy/sharedb-ace

@RichDom2185
Copy link
Member

@TheMythologist sorry for the delay, there's still a type error on UseShareAce (even though last I checked using a locally-linked version, everything was fine). Could you check?

@TheMythologist
Copy link
Contributor Author

Hi, seems like events-emitter-es6 was causing problems. Switched that package for node:events. Created a PR fix here: source-academy/sharedb-ace#32

Let me know once the new package is published, I will bump the version of @sourceacademy/sharedb-ace and hopefully there won't be any more type errors

@TheMythologist
Copy link
Contributor Author

Hi sorry to bother Prof @martin-henz, but could you help to bump the @sourceacademy/sharedb-ace again? Bump it to 2.1.1 (it's already updated in the package.json file)

@martin-henz
Copy link
Member

Hi sorry to bother Prof @martin-henz, but could you help to bump the @sourceacademy/sharedb-ace again? Bump it to 2.1.1 (it's already updated in the package.json file)

OK done.

@TheMythologist
Copy link
Contributor Author

Sorry prof @martin-henz, requesting you to bump the @sourceacademy/sharedb-ace again one last time, to 2.1.2 (it's already updated in the package.json file).

@TheMythologist
Copy link
Contributor Author

Sorry prof @martin-henz, bumping again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@salmonkarp salmonkarp salmonkarp left review comments

@RichDom2185 RichDom2185 Awaiting requested review from RichDom2185

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TheMythologist @RichDom2185 @martin-henz @salmonkarp