Yuqiang/student dashboard loading issue #8016
Conversation
WalkthroughThe changes involve syntactic adjustments to object literals in several files, restructuring of template logic for rendering course instances and next level buttons, and an optimization in session fetching by grouping course instances by language and course ID. No exported or public entity signatures are altered. Changes
Sequence Diagram(s)sequenceDiagram
participant Teacher as TeacherClassesView
participant CourseInstance
participant CoursesView
participant SessionsCollection
Teacher->>CourseInstance: Create new instance (with aceConfig)
CoursesView->>CourseInstance: Group by language & course ID
loop For each group
CoursesView->>SessionsCollection: Fetch sessions (per language/course)
SessionsCollection-->>CourseInstance: Assign sessions to all in group
end
CoursesView->>CoursesView: Render course-instance entries
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn EBADENGINE Unsupported engine { Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/core/store/modules/courseInstances.js (1)
156-159:aceConfigobject is re-created on every invocation – consider reuse / merge.
aceConfigis now initialised with{ language: classroom.aceConfig?.language }.
Two follow-ups:
languagemay still beundefinedfor legacy classrooms. In that case we end up persisting{ language: undefined }, which clutters the document without adding value.- If more fields are later added to
classroom.aceConfig, they will be silently dropped here because we always build a fresh object instead of cloning / shallow-merging.- aceConfig: { - language: classroom.aceConfig?.language, - } + // Preserve any existing aceConfig keys and only overwrite missing values + aceConfig: { + ...classroom.aceConfig, + language: classroom.aceConfig?.language ?? null + }app/views/courses/TeacherClassView.js (1)
775-778: Potentially undefinedthis.classroom.language.
this.classroom.languageis only set once the classroom finishes syncing (line 113).
If a teacher hits “Assign” before the firstsyncevent returns,languagewill beundefinedand we store{ language: undefined }inside the instance, mirroring the issue mentioned in the store module.Either gate the call until the classroom is synced or fall back to
this.classroom.get('aceConfig')?.language.app/views/courses/CoursesView.js (1)
374-383: Share one fetched collection, but beware of mutation.All instances in a (language, course) bucket receive the exact same
collectionreference.
If any code later mutatesinstance.sessions.models(e.g., viareset), it mutates them for every sibling instance.
Add a comment or freeze the collection to avoid surprising side-effects.app/templates/courses/courses-view.pug (1)
304-337: Dynamic ‘play next level’ tile lacks click handler when disabled.When
nextLevelLockedis true the anchor is rendered withdisabledattribute but still as<a>. Browsers don’t natively honourdisabledon anchors – the link remains focusable / clickable.Consider:
-if view.nextLevelInfo - a.play-next-level-btn.tile(disabled=nextLevelLocked … +if view.nextLevelInfo + - var tag = nextLevelLocked ? 'div' : 'a' + #{tag}.play-next-level-btn.tile(class=nextLevelLocked ? 'disabled' : '', href=nextLevelLocked ? null : view.nextLevelUrl())Or attach
pointer-events: nonevia CSS to.disabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/core/store/modules/courseInstances.js(1 hunks)app/templates/courses/courses-view.pug(3 hunks)app/views/courses/CoursesView.js(3 hunks)app/views/courses/TeacherClassView.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ESLint CI
🔇 Additional comments (1)
app/templates/courses/courses-view.pug (1)
181-218: Indentation / flow control may break Pug compile.Inside the
if nextLevelblock the subsequentif view.utils.isCodeCombatline is preceded by an extra space (␠if). Pug treats leading spaces as indentation; mis-alignment can raise a “Block in mixin expected” error.Verify that this chunk compiles:
if nextLevel - var isNextLevelLocked = … … if view.utils.isCodeCombat +course-instance-body(…) else +course-instance-body(…)Remove stray spa 8000 ces before control keywords.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
app/views/courses/ClassroomView.js (1)
115-124:⚠️ Potential issueIncorrect reference:
sessionspoints to the wrong collectionInside the JIT course-instance generation block, the line
sessions.courseInstance = courseInstancemutates the last
sessionscollection created in the previous loop, not the brand-new empty collection you just assigned tocourseInstance.sessions. This silently links two unrelated objects and will surface as confusing bugs (wrong courseInstance on a session, duplicated listeners, etc.).Patch:
- courseInstance.sessions = new CocoCollection([], { model: LevelSession }) - sessions.courseInstance = courseInstance + courseInstance.sessions = new CocoCollection([], { model: LevelSession }) + courseInstance.sessions.courseInstance = courseInstanceapp/views/courses/TeacherClassesView.js (1)
261-263: 🛠️ Refactor suggestionExisting promise construction is fragile
Not introduced in this diff, but while you’re here:
promises.push(new Promise(courseInstance.save(...).then))creates aPromisewith an executor equal to whatever.thenreturns (likelyundefined)—this is a no-op and breaks error handling. Prefer pushing thejqXHR(which is a thenable) or wrap it explicitly:- promises.push(new Promise(courseInstance.save(null, { validate: false }).then)) + promises.push(courseInstance.save(null, { validate: false }))
🧹 Nitpick comments (1)
app/views/courses/TeacherClassesView.js (1)
254-259: Language config added, but watch the trailing commaThe new
aceConfigblock is correct; however, the trailing comma after thelanguageproperty producesaceConfig: { language: undefined, }when the classroom lacks the key. While harmless in most engines, we don’t use trailing commas elsewhere in this file—drop it for consistency.- aceConfig: { - language: classroom.get('aceConfig')?.language, - }, + aceConfig: { + language: classroom.get('aceConfig')?.language + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/views/courses/ClassroomView.js(1 hunks)app/views/courses/TeacherClassesView.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/views/courses/ClassroomView.js (1)
app/views/courses/ 8000 TeacherClassesView.js (2)
courseInstance(251-251)CourseInstance(26-26)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ESLint CI
- GitHub Check: Node.js CI (20.19.2)
🔇 Additional comments (1)
app/views/courses/ClassroomView.js (1)
115-120: 👍 Consistent propagation of language viaaceConfigAdding the classroom’s
aceConfig.languagewhen synthesising aCourseInstancekeeps the object graph consistent with the rest of the refactor and removes several runtime look-ups. Looks good.
fix ENG-1865
optimize by 2 steps.
do not wait for level sessions fetch. actually progress is not so important in student page. the most important thing in student home page is button/link to the course-campaign page. we fetch the session in campaign page again. So if the network is slow, we should allow student to access campaign page without waiting for all course-sessions.
example page when sessions is not loaded:
when fething instance-sessions, we actually share progress for same language classrooms. so if courseID and language is same, we don't need fetch sessions twice. consider level.session is slowest, reduce even 1 request is better. Esp in china some student join many 'python' classrooms for custom tournaments. (we do not have esports classroom before).
Summary by CodeRabbit