default to blocks-icons for codecombat-junior #7839
Conversation
WalkthroughThe changes in the Changes
Assessment against linked issues
Possibly related PRs
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 (1.23.1)
warning eslint@8.57.1: This version is no longer supported. Please see https://eslint.org/version-support for other options. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Outside diff range and nitpick comments (1)
app/views/play/level/tome/TomeView.coffee (1)
121-124: LGTM! Consider improving readability with a comment.The implementation correctly ensures that CodeCombat Junior defaults to block mode while preserving user preferences. The precedence order is clear:
- User's preferred format
- 'blocks-icons' for CodeCombat Junior
- Classroom default format
Consider adding a brief comment explaining the precedence order for future maintainers:
desiredCodeFormat = me.get('aceConfig')?.codeFormat + # Format precedence: user preference > product default > classroom default if @options.level.get('product') === 'codecombat-junior' desiredCodeFormat ?= 'blocks-icons' desiredCodeFormat ?= classroomCodeFormatDefault
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/views/play/level/tome/TomeView.coffee(1 hunks)
🔇 Additional comments (1)
app/views/play/level/tome/TomeView.coffee (1)
121-124: Verify the impact on existing tests and configurations.
Let's ensure this change is properly covered by tests and doesn't conflict with existing configurations.
✅ Verification successful
Let me gather more specific information about how the code format is determined in these files to ensure there are no conflicts.
Based on the comprehensive results, I can now provide a final response about the code format changes:
Changes are consistent with existing configurations and test coverage
The modification to set blocks-icons as the default format for CodeCombat Junior is well-supported by the codebase:
- The schema in
classroom.schema.jsproperly definesblocks-iconsas a valid option in thecodeFormatDefaultenum - The Vue component
CourseCodeLanguageFormatComponenthandlesblocks-iconsformat appropriately, including proper disabling logic for non-Junior courses - The constants in
core/constants.jsincludeblocks-iconsin bothCODE_FORMAT_ALLandCODE_FORMAT_IPADarrays - The change aligns with the existing classroom configuration system where code formats can be controlled at both classroom and user levels
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related code and test coverage
# Check for other references to blocks-icons and code format determination
echo "Checking for related code..."
rg -l "blocks-icons"
# Check for test files that might need updating
echo "Checking for related test files..."
fd -e test.coffee -e spec.coffee | xargs rg -l "codeFormat|blocks-icons"
# Check for classroom configuration files
echo "Checking for classroom config files..."
fd -e config.coffee | xargs rg -l "codeFormat|blocks-icons"
Length of output: 2557
Script:
#!/bin/bash
# Check the specific implementation details around code format determination
# Look for code format determination logic
echo "Checking code format determination logic..."
rg -A 5 "codeFormat.*=|get\('codeFormat'\)"
# Check classroom schema for default values
echo "Checking classroom schema defaults..."
cat app/schemas/models/classroom.schema.js
# Check constants related to code format
echo "Checking code format constants..."
rg -A 2 "CODE_FORMAT|codeFormat" app/core/constants.js
# Check for any potential conflicts in Vue components
echo "Checking Vue component logic..."
cat "ozaria/site/components/teacher-dashboard/modals/modal-edit-class-components/CourseCodeLanguageFormatComponent.vue"
Length of output: 47115
| desiredCodeFormat = me.get('aceConfig')?.codeFormat | ||
| if @options.level.get('product') == 'codecombat-junior' | ||
| desiredCodeFormat ?= 'blocks-icons' | ||
| desiredCodeFormat ?= classroomCodeFormatDefault | ||
| newCodeFormat = if desiredCodeFormat in classroomCodeFormats then desiredCodeFormat else classroomCodeFormatDefault |
There was a problem hiding this comment.
I'm not sure what is the objective here, but this line will reassign it back to text-code if classroomCodeFormats doesn't contain block-icons
There was a problem hiding this comment.
ya it is. if the teacher want classroom don't use block they can still disable that so that junior will still use text-code
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
ozaria/site/components/teacher-dashboard/BaseCurriculumGuide/curriculum-guide-helper.js (1)
7-11: Consider handling existing codeFormat parameter.The current implementation might overwrite an existing codeFormat parameter if one is present in the URL. Consider checking if the URL already contains a codeFormat parameter before appending a new one.
Here's a suggested improvement:
- let url = `/play/level/${slug}?course=${courseId}&codeLanguage=${codeLanguage}` - if (courseId === utils.courseIDs.JUNIOR) { - url += '&codeFormat=blocks-icons' - } + const params = new URLSearchParams({ + course: courseId, + codeLanguage: codeLanguage + }); + if (courseId === utils.courseIDs.JUNIOR && !params.has('codeFormat')) { + params.set('codeFormat', 'blocks-icons'); + } + let url = `/play/level/${slug}?${params.toString()}`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/views/play/level/tome/TomeView.coffee(1 hunks)ozaria/site/components/teacher-dashboard/BaseCurriculumGuide/curriculum-guide-helper.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/play/level/tome/TomeView.coffee
🔇 Additional comments (1)
ozaria/site/components/teacher-dashboard/BaseCurriculumGuide/curriculum-guide-helper.js (1)
7-11: Implementation looks good, verify courseIDs constant.
The implementation correctly adds the blocks-icons format for Junior courses, aligning with the PR objective.
Let's verify the courseIDs constant is properly defined:
✅ Verification successful
Implementation verified, courseIDs.JUNIOR is properly defined
The courseIDs.JUNIOR constant is correctly defined in app/core/utils.js with a valid ID value ('65f32b6c87c07dbeb5ba1936'). The implementation in curriculum-guide-helper.js can safely rely on this constant.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify JUNIOR course ID is defined
rg -A 5 "courseIDs.*=.*{" app/core/utils
Length of output: 136
Script:
#!/bin/bash
# Search for courseIDs definition with proper regex escaping
rg -A 5 "courseIDs\s*=\s*\{" app/core/utils
# Also try searching in other common locations
rg -A 5 "courseIDs\s*=\s*\{" app/core
rg -A 5 "courseIDs\s*=\s*\{" ozaria/site
# Search specifically for JUNIOR constant
rg "JUNIOR.*:" --type js
Length of output: 1496
| @@ -118,8 +118,15 @@ module.exports = class TomeView extends CocoView | |||
| else | |||
| classroomCodeFormatDefault = @options.classroomAceConfig?.codeFormatDefault or 'text-code' | |||
| classroomCodeFormats = @options.classroomAceConfig?.codeFormats or ['blocks-icons', 'blocks-text', 'blocks-and-code', 'text-code'] | |||
| desiredCodeFormat = me.get('aceConfig')?.codeFormat or classroomCodeFormatDefault | |||
| desiredCodeFormat = me.get('aceConfig')?.codeFormat | |||
| if @options.level.get('product') == 'codecombat-junior' | |||
There was a problem hiding this comment.
do we need these changes with query variable below now?
There was a problem hiding this comment.
these changes still work for new users. query variable now only work for teacher-dashboard. i think we can keep them until we make new logic for codeformat
fix ENG-1418
Summary by CodeRabbit
New Features
Bug Fixes
Chores