Stage 2 prototype #2437
Stage 2 prototype #2437
Conversation
WalkthroughThe pull request introduces several changes to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TutorialCard
participant ImplementSolutionStep
participant LanguageGuideCard
User->>TutorialCard: Expand step
TutorialCard->>ImplementSolutionStep: Render solution
alt Course slug is "shell"
ImplementSolutionStep->>User: Show instructions
end
alt Language guide present
ImplementSolutionStep->>LanguageGuideCard: Render language guide
end
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 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 (
|
Bundle ReportChanges will decrease total bundle size by 2.52kB (-0.01%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
Test Results 1 files ±0 1 suites ±0 7m 11s ⏱️ -14s Results for commit f4db679. ± Comparison against base commit b7ee6d1. This pull request removes 1 test.♻️ This comment has been updated with latest results. |
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
app/components/course-page/course-stage-step/second-stage-tutorial-card.hbs (1)
Line range hint
8-13: Consider implementing dynamic statistics fetchingThe hardcoded 98% completion rate should be replaced with actual data from the backend as noted in the TODO comment.
Would you like me to help create a GitHub issue to track the implementation of dynamic statistics fetching from the backend? This could include:
- API endpoint design for aggregating stage completion rates
- Frontend integration code
- Loading state handling
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs (2)
1-7: Consider consolidating instructions into markdown contentThe static paragraph "Head over to your editor / IDE..." could potentially be moved into
@courseStage.shortInstructionsMarkdownfor better maintainability and consistency.<div class="prose dark:prose-invert prose-compact mb-3"> {{markdown-to-html @courseStage.shortInstructionsMarkdown}} - - <p> - Head over to your editor / IDE and implement your solution. - </p> </div>
Line range hint
8-45: Enhance accessibility and optimize conditional checksThe solution display section has a few areas for improvement:
- The reveal solution button lacks proper ARIA attributes
- The double condition check for
this.solutionis redundantConsider these improvements:
<SecondaryButton + aria-label="Reveal solution" class="self-center bg-white group-hover:bg-teal-50 dark:bg-gray-900 dark:group-hover:bg-gray-900/80 backdrop-blur-3xl group-hover:text-teal-600 dark:group-hover:text-teal-400 group-hover:border-teal-600 dark:group-hover:border-teal-400" data-test-reveal-solution-button >And simplify the nested condition:
-{{#if this.solution}} <FileDiffCard @filename={{changedFile.filename}} @code={{changedFile.diff}} @language={{this.solution.language.slug}} data-test-file-diff-card /> -{{/if}}app/components/expandable-step-list/step.hbs (1)
Line range hint
39-46: LGTM! Consider adding ARIA attributes for better accessibility.The conditional rendering logic for the collapse button is well-structured and aligns with the UI simplification goals.
Consider adding ARIA attributes to improve accessibility:
- {{#if (and @isExpanded @step.isComplete (not @nextIncompleteStep))}} - <TertiaryButton @size="extra-small" {{on "click" @onCollapse}}> + {{#if (and @isExpanded @step.isComplete (not @nextIncompleteStep))}} + <TertiaryButton + @size="extra-small" + aria-expanded="true" + aria-label="Collapse current step" + {{on "click" @onCollapse}}> <div class="flex items-center gap-1"> {{svg-jar "arrow-up" class="w-3 h-3"}} <span>Collapse</span> </div> </TertiaryButton> {{/if}}app/models/course-stage.ts (2)
145-156: Add TypeScript return type and improve string template formatting.Consider adding a return type annotation and improving the template string formatting:
- get shortInstructionsMarkdown() { + get shortInstructionsMarkdown(): string { return `In this stage, you'll implement support for handling missing commands in your shell. Example: \`\`\` $ missing_command missing_command: not found \`\`\` `; }
145-156: Consider using template variables for consistency.The
prerequisiteInstructionsMarkdownTemplatemethod uses Mustache templates for dynamic content. For consistency and future extensibility, consider using the same approach here to support language-specific examples or customizations.Example approach:
get shortInstructionsMarkdown(): string { const template = ` In this stage, you'll implement support for handling missing commands in your shell. Example: \`\`\` {{#command_prefix}}$ {{/command_prefix}}missing_command {{error_prefix}}missing_command: not found{{/error_prefix}} \`\`\` `; return Mustache.render(template, { command_prefix: true, error_prefix: true }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
app/components/course-page/course-stage-step/second-stage-tutorial-card.hbs(1 hunks)app/components/course-page/course-stage-step/second-stage-tutorial-card.ts(0 hunks)app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs(2 hunks)app/components/expandable-step-list/step.hbs(1 hunks)app/models/course-stage.ts(1 hunks)
💤 Files with no reviewable changes (1)
- app/components/course-page/course-stage-step/second-stage-tutorial-card.ts
🔇 Additional comments (2)
app/components/course-page/course-stage-step/second-stage-tutorial-card.hbs (1)
Line range hint 16-24: Verify UX impact of removing ReadInstructionsStep
The simplified flow looks good, but please ensure:
- New users still have sufficient guidance without the explicit instructions step
- The
ImplementSolutionStepcomponent receives all necessary props for proper functionality
Let's verify the component's props and related files:
✅ Verification successful
Let me gather more information about the ImplementSolutionStep component and its usage.
Let me gather more information about the ImplementSolutionStep component's template and its usage.
Based on the gathered information, I can now generate the final response:
Removal of ReadInstructionsStep is safe and UX is maintained
The ImplementSolutionStep component effectively maintains user guidance by:
- Displaying concise instructions via
courseStage.shortInstructionsMarkdown - Providing clear direction to implement the solution
- Offering an optional solution reveal feature with proper UX controls
All necessary props are properly typed and passed through:
@repository@courseStage@isComplete@shouldRecommendLanguageGuide@shouldShowSolution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to ReadInstructionsStep and verify ImplementSolutionStep props
# Check for any lingering references to ReadInstructionsStep
rg "ReadInstructionsStep"
# Find the ImplementSolutionStep component definition to verify props
ast-grep --pattern 'class ImplementSolutionStep extends Component {
$$$
}'
Length of output: 503
Script:
#!/bin/bash
# Search for ImplementSolutionStep component definition and template
fd "implement-solution-step" --type f
# Check the content of these files
rg -A 5 "ImplementSolutionStep" --type ts
rg -A 5 "@repository|@courseStage|@isComplete|@shouldRecommendLanguageGuide|@shouldShowSolution" --type hbs
Length of output: 72702
Script:
#!/bin/bash
# Check the template content of ImplementSolutionStep
cat app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs
# Check for any references to ReadInstructionsStep in the component's history
rg -A 5 "ReadInstructionsStep" --type ts --type hbs
# Check the component's TypeScript definition for props
cat app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.ts
Length of output: 4245
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs (1)
46-53: Remove commented-out code
The commented sections should either be:
- Implemented if they represent planned features
- Removed if they're no longer needed
Keeping commented-out code can lead to confusion and maintenance issues.
Let's check if these features are tracked elsewhere:
✅ Verification successful
Based on the search results, I can generate the final response:
Remove commented-out code sections
The commented-out sections should be removed as they appear to be obsolete UI elements:
- The "Show explanation" button functionality is already implemented in other components (like
concept-page/question-card.hbs), but using a different design pattern. - The completion instruction text is redundant as similar instructions exist in
read-instructions-step.hbs.
These commented sections are not part of any active feature development and should be cleaned up to maintain code clarity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related feature discussions or issues
gh issue list --search "explanation button course stage" --state all
gh issue list --search "step completion instructions" --state all
Length of output: 138
Script:
#!/bin/bash
# Let's search for similar UI elements or related code patterns in the codebase
rg -l "Show explanation|explanation button" --type hbs --type js
rg -l "Mark this step as complete" --type hbs --type js
# Check if these components are used elsewhere
rg "TertiaryButton" --type hbs --type js -A 2
Length of output: 37518
andy1li
force-pushed
the
stage-2-prototype
branch
from
7bc14fd to
5ba7669
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
app/components/course-page/course-stage-step/embedded-language-guide-card.hbs (2)
3-15: Consider extracting dark mode styles into CSS custom properties.While the dark mode styling is comprehensive, the inline classes make it harder to maintain consistent theming across components.
Consider refactoring the dark mode styles like this:
-class="flex justify-center items-center gap-1 dark:bg-transparent dark:text-gray-200 dark:border-white/5 dark:hover:border-gray-700/60 dark:bg-gray-800 dark:hover:bg-gray-700/50" +class="flex justify-center items-center gap-1 {{style-tokens 'tertiary-button-dark'}}"Then define the styles in a central theme file:
/* app/styles/theme.css */ .tertiary-button-dark { --bg-color: transparent; --text-color: rgb(229 231 235); /* gray-200 */ --border-color: rgb(255 255 255 / 0.05); --hover-border-color: rgb(55 65 81 / 0.6); --hover-bg-color: rgb(55 65 81 / 0.5); }
17-21: Consider handling loading states for markdown content.The markdown content might take time to render, especially with code highlighting. Consider adding a loading state to improve user experience.
Here's a suggested implementation:
{{#if this.explanationIsVisible}} + {{#if this.isLoading}} + <div class="animate-pulse h-32 bg-gray-100 dark:bg-gray-800 rounded"></div> + {{else}} <div class="prose dark:prose-invert has-prism-highlighting" {{highlight-code-blocks @languageGuide.markdownForBeginner}}> {{markdown-to-html @languageGuide.markdownForBeginner}} </div> + {{/if}} {{/if}}app/components/course-page/course-stage-step/embedded-language-guide-card.ts (1)
1-31: Consider architectural improvements for better user experienceAs this is part of the Stage 2 prototype, consider these enhancements:
- Persist the explanation visibility state in localStorage to maintain user preference across sessions
- Add analytics tracking to understand guide usage patterns and improve the feature
Would you like me to provide implementation details for these suggestions?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 16-16: app/components/course-page/course-stage-step/embedded-language-guide-card.ts#L16
Added line #L16 was not covered by tests
[warning] 21-21: app/components/course-page/course-stage-step/embedded-language-guide-card.ts#L21
Added line #L21 was not covered by tests
[warning] 24-24: app/components/course-page/course-stage-step/embedded-language-guide-card.ts#L24
Added line #L24 was not covered by testsapp/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.ts (1)
Line range hint
20-33: Consider adding languageGuide-related functionalityThe component doesn't currently utilize the new
languageGuideproperty in its TypeScript logic. Consider whether any additional functionality should be added to handle language guide-specific behaviors.For example, you might want to add:
get hasLanguageGuide(): boolean { return !!this.args.languageGuide; }app/components/expandable-step-list.ts (2)
Line range hint
71-77: Consider adding error boundaries and configurable scroll behavior.The
#expandStepAndScrollContainerIntoViewmethod could be enhanced with:
- Error boundaries for cases where the container element is not available
- Configurable scroll behavior options passed through component arguments
Consider applying these improvements:
interface Signature { Args: { onManualStepComplete?: (step: Step) => void; steps: Step[]; stepContainerClass?: string; + scrollBehavior?: ScrollBehavior; }; // ... } #expandStepAndScrollContainerIntoView(step: Step) { this.expandedStepId = step.id; if (this.containerElement) { - this.containerElement.scrollIntoView({ behavior: 'smooth' }); + try { + this.containerElement.scrollIntoView({ + behavior: this.args.scrollBehavior ?? 'smooth' + }); + } catch (error) { + console.warn('Failed to scroll container into view:', error); + } } }
Line range hint
5-11: Enhance type safety and interface definitions.The
Stepinterface could be more robust with additional type safety measures.Consider these type safety improvements:
export interface Step { id: string; titleMarkdown: string; isComplete: boolean; canBeCompletedManually: boolean; + // Add readonly to prevent accidental mutations + readonly id: string; + readonly titleMarkdown: string; + // Add optional properties that might be needed based on usage + description?: string; + // Add union type for potential step states + status: 'not_started' | 'in_progress' | 'completed'; }app/components/course-page/course-stage-step/second-stage-tutorial-card.ts (2)
Line range hint
66-71: Critical: Clean up remaining references to removed ReadInstructionsStepThe code contains several inconsistencies after the removal of
ReadInstructionsStep:
readInstructionsStepIsCompleteandreadInstructionsStepWasMarkedAsCompletemethods are now unusedhandleStepCompletedManuallystill processes the 'read-instructions' step IDApply this cleanup:
- get readInstructionsStepIsComplete() { - return this.implementSolutionStepIsComplete || this.readInstructionsStepWasMarkedAsComplete; - } - get readInstructionsStepWasMarkedAsComplete() { - return this.coursePageState.manuallyCompletedStepIdsInSecondStageInstructions.includes('read-instructions'); - } @action handleStepCompletedManually(step: Step) { - if (step.id === 'read-instructions') { - this.coursePageState.recordManuallyCompletedStepInSecondStageInstructions('read-instructions'); - - this.analyticsEventTracker.track('completed_second_stage_tutorial_step', { - step_number: 1, - step_id: 'read-instructions', - repository_id: this.args.repository.id, - }); - } if (step.id === 'implement-solution') { - this.coursePageState.recordManuallyCompletedStepInSecondStageInstructions('read-instructions'); this.coursePageState.recordManuallyCompletedStepInSecondStageInstructions('implement-solution'); this.analyticsEventTracker.track('completed_second_stage_tutorial_step', { - step_number: 2, + step_number: 1, step_id: 'implement-solution', repository_id: this.args.repository.id, }); } }Also applies to: 76-78, 93-102
Line range hint
23-54: Consider extracting step management to a dedicated serviceThe current implementation manages steps through individual classes and component methods. Consider extracting this logic into a dedicated
StepManagerservice to:
- Centralize step management logic
- Make it easier to add/remove steps
- Reduce component complexity
- Make step state management more maintainable
Example structure:
// services/step-manager.ts class StepManager { constructor(repository: RepositoryModel) { this.repository = repository; } registerStep(step: BaseStep) { ... } isStepComplete(stepId: string) { ... } handleStepCompletion(stepId: string) { ... } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
app/components/course-page/course-stage-step/embedded-language-guide-card.hbs(1 hunks)app/components/course-page/course-stage-step/embedded-language-guide-card.ts(1 hunks)app/components/course-page/course-stage-step/second-stage-tutorial-card.hbs(2 hunks)app/components/course-page/course-stage-step/second-stage-tutorial-card.ts(2 hunks)app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs(2 hunks)app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.ts(2 hunks)app/components/expandable-step-list.ts(1 hunks)app/components/expandable-step-list/step.hbs(1 hunks)app/controllers/course/stage/instructions.ts(0 hunks)app/templates/course/stage/instructions.hbs(1 hunks)
💤 Files with no reviewable changes (1)
- app/controllers/course/stage/instructions.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/components/course-page/course-stage-step/second-stage-tutorial-card.hbs
- app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs
- app/components/expandable-step-list/step.hbs
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/components/course-page/course-stage-step/embedded-language-guide-card.ts
[warning] 16-16: app/components/course-page/course-stage-step/embedded-language-guide-card.ts#L16
Added line #L16 was not covered by tests
[warning] 21-21: app/components/course-page/course-stage-step/embedded-language-guide-card.ts#L21
Added line #L21 was not covered by tests
[warning] 24-24: app/components/course-page/course-stage-step/embedded-language-guide-card.ts#L24
Added line #L24 was not covered by tests
🔇 Additional comments (8)
app/components/course-page/course-stage-step/embedded-language-guide-card.hbs (1)
1-2: LGTM! Well-structured container layout.
The grid layout with appropriate spacing and test attributes provides a solid foundation for the component.
app/components/course-page/course-stage-step/embedded-language-guide-card.ts (2)
1-12: LGTM! Well-structured type definitions
The imports and interface definitions are clean and properly typed, following TypeScript best practices.
27-31: LGTM! Proper Glint type registry setup
The type registry augmentation is correctly implemented, ensuring proper type checking in templates.
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.ts (2)
4-4: LGTM: Import statement is properly structured
The import for CourseStageLanguageGuideModel is correctly placed and follows the project's conventions.
15-15: Verify template updates for optional languageGuide property
The change from shouldRecommendLanguageGuide boolean to optional languageGuide model suggests a more data-driven approach. Please ensure all template conditions have been updated accordingly.
✅ Verification successful
Interface change has been properly implemented
The verification shows that:
- The old
shouldRecommendLanguageGuideboolean is completely removed with no remaining references - The new
languageGuideproperty is properly:- Defined in the interface with correct type
- Used correctly in the template with conditional rendering
- Passed to the appropriate
EmbeddedLanguageGuideCardcomponent
- Related components (
EmbeddedLanguageGuideCard,SimpleLanguageGuideCard) are properly typed to handle theCourseStageLanguageGuideModel
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to shouldRecommendLanguageGuide
# and verify the new languageGuide property usage
# Check for any remaining references to the old property
rg "shouldRecommendLanguageGuide" --type hbs
# Check usage of the new languageGuide property in templates
rg "languageGuide" --type hbs -A 3 app/components/course-page/course-stage-step/second-stage-tutorial-card/
Length of output: 696
Script:
#!/bin/bash
# Let's verify the component implementation to ensure proper handling
ast-grep --pattern 'class ImplementSolutionStep {
$$$
}' app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.ts
# Check for any test files that might need updates
fd "implement-solution-step-test" --exec cat {}
Length of output: 209
Script:
#!/bin/bash
# Let's check the component file content directly
cat app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.ts
# Also check for any related component that might use this property
rg -t ts "languageGuide.*CourseStageLanguageGuideModel" -A 2
Length of output: 2990
app/components/expandable-step-list.ts (1)
40-42: Verify the impact of simplified initialization logic.
The initialization logic has been simplified to always expand the first incomplete step. While this change makes the code cleaner, we should verify that removing the comparison check doesn't affect the expected behavior in edge cases.
Let's verify the usage of this component to ensure the change doesn't cause issues:
✅ Verification successful
Based on the gathered information, I can now provide the final response:
The simplified initialization logic is safe and correct.
The change to always expand the first incomplete step is the right approach because:
- The component's template shows that non-complete steps before the first incomplete step are rendered as non-expandable (
<ExpandableStepList::NonExpandableStep>) - The
firstIncompleteStepis used consistently throughout the component to control step expansion and visibility - The step expansion logic maintains proper state through
handleStepCollapseand other related methods
The removal of the additional comparison check simplifies the code without compromising functionality, as the component's structure ensures that only the appropriate steps can be expanded.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all instances where ExpandableStepList is used to understand the context
# and potential impact of the initialization change
# Search for component usage in templates
echo "Component usage in templates:"
rg -A 5 "ExpandableStepList"
# Search for test files to understand expected behavior
echo "\nRelated test files:"
fd -e ts -e js "expandable-step-list-test"
Length of output: 5156
Script:
#!/bin/bash
# Let's examine the test files and implementation to understand the behavior better
# Search for test files in the entire repository
echo "Test files content:"
fd -e ts -e js "test" | rg -l "expandable-step-list" | xargs cat
# Look for the step interface/type definition to understand step structure
echo "\nStep interface/type definition:"
rg -A 10 "interface.*Step.*{" || rg -A 10 "type.*Step.*="
# Check for any related component state management
echo "\nComponent state management:"
ast-grep --pattern 'expandedStepId = $_'
Length of output: 6312
Script:
#!/bin/bash
# Let's examine the component's implementation and usage more closely
# Look for step status handling and expansion logic
echo "Step status and expansion handling:"
rg -A 5 "firstIncompleteStep"
# Check for any step expansion/collapse related methods
echo "\nStep expansion/collapse methods:"
ast-grep --pattern 'handleStep$_($$$) {
$$$
}'
# Look for the component's interface/type definitions
echo "\nComponent interface:"
rg -A 10 "interface.*Signature.*{" || rg -A 10 "type.*Signature.*="
Length of output: 7255
app/components/course-page/course-stage-step/second-stage-tutorial-card.ts (1)
7-7: LGTM: Clean type import addition
The type-only import for CourseStageLanguageGuideModel is properly added and follows TypeScript best practices.
app/templates/course/stage/instructions.hbs (1)
43-43: LGTM! Improved data flow for language guide handling.
The change from @shouldRecommendLanguageGuide to @languageGuide represents a better pattern by directly passing the guide data instead of a boolean flag.
Let's verify the SecondStageTutorialCard component handles the new parameter correctly:
✅ Verification successful
Change from boolean flag to guide object is properly handled
The template shows that @languageGuide is correctly passed down to the ImplementSolutionStep component, which is the actual consumer of the language guide data. The change from a boolean flag to passing the actual guide object represents a cleaner data flow pattern.
- The parent template (
instructions.hbs) properly passes@languageGuide - The
SecondStageTutorialCardcomponent correctly forwards it toImplementSolutionStep - No conditional logic around the guide is needed at this level anymore
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SecondStageTutorialCard component's language guide handling
# Expected: Component template should use @languageGuide parameter
# Search for the component template
fd -e hbs -e js "second-stage-tutorial-card"
# Check usage of languageGuide in component
ast-grep --pattern 'export default class SecondStageTutorialCard extends Component {
$$$
@tracked languageGuide;
$$$
}'
Length of output: 248
Script:
#!/bin/bash
# Let's check the component template and its usage of languageGuide
cat app/components/course-page/course-stage-step/second-stage-tutorial-card.hbs
# Also check the component class implementation
rg -g "*.js" "class SecondStageTutorialCard" -A 10
Length of output: 1689
Script:
#!/bin/bash
# Let's check the JS component and its usage of languageGuide
ast-grep --pattern 'class SecondStageTutorialCard extends Component {
$$$
}'
# Also check the implementation of ImplementSolutionStep component since it receives the languageGuide
fd -e js "implement-solution-step"
Length of output: 115
app/components/course-page/course-stage-step/embedded-language-guide-card.hbs
Outdated
Show resolved
Hide resolved
app/components/course-page/course-stage-step/embedded-language-guide-card.ts
Outdated
Show resolved
Hide resolved
app/components/course-page/course-stage-step/embedded-language-guide-card.ts
Outdated
Show resolved
Hide resolved
app/components/course-page/course-stage-step/second-stage-tutorial-card.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
tests/acceptance/course-page/language-guides-test.js (2)
55-65: Add assertion for revealed solution stateWhile the toggle explanation functionality is well tested, there's no assertion to verify the state after clicking the reveal solution button. Consider adding an assertion to ensure the solution is properly revealed.
await coursePage.secondStageTutorialCard.clickOnRevealSolutionButton(); +assert.ok( + coursePage.secondStageTutorialCard.isSolutionVisible, + 'Solution should be visible after clicking reveal button' +); await coursePage.languageGuideCard.clickOnToggleExplanationButton();
Line range hint
13-66: Consider adding test coverage for edge casesThe current test covers the happy path, but consider adding test cases for:
- Attempting to view language guides when none exist
- Toggling explanation multiple times
- Interaction between solution reveal and explanation toggle
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
app/components/course-page/course-stage-step/embedded-language-guide-card.hbs(1 hunks)tests/acceptance/course-page/complete-first-stage-test.js(0 hunks)tests/acceptance/course-page/complete-second-stage-test.js(0 hunks)tests/acceptance/course-page/complete-stage-without-changes-test.js(0 hunks)tests/acceptance/course-page/course-stage-solutions-test.js(0 hunks)tests/acceptance/course-page/language-guides-test.js(2 hunks)tests/pages/components/course-page/course-stage-step/second-stage-tutorial-card.ts(0 hunks)tests/pages/course-page.js(1 hunks)
💤 Files with no reviewable changes (5)
- tests/acceptance/course-page/complete-first-stage-test.js
- tests/acceptance/course-page/complete-second-stage-test.js
- tests/acceptance/course-page/complete-stage-without-changes-test.js
- tests/acceptance/course-page/course-stage-solutions-test.js
- tests/pages/components/course-page/course-stage-step/second-stage-tutorial-card.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/course-page/course-stage-step/embedded-language-guide-card.hbs
🔇 Additional comments (2)
tests/acceptance/course-page/language-guides-test.js (1)
27-36: LGTM: Well-structured test fixture setup
The course-stage-solution creation is properly configured with appropriate relationships and a well-formatted code diff.
tests/pages/course-page.js (1)
168-168: LGTM! The test helper follows established patterns.
The new clickOnToggleExplanationButton test helper is well-placed within the languageGuideCard object and follows the established naming conventions and selector patterns used throughout the file.
andy1li
force-pushed
the
stage-2-prototype
branch
from
efa1dd1 to
acfa31b
Compare
andy1li
force-pushed
the
stage-2-prototype
branch
from
acfa31b to
9695dcf
Compare
andy1li
force-pushed
the
stage-2-prototype
branch
from
9695dcf to
be5098c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tests/acceptance/course-page/language-guides-test.js (1)
27-35: Consider moving the diff content to a fixture file.The inline diff makes the test less readable and harder to maintain. Consider moving it to a fixture file in the
tests/fixturesdirectory.- this.server.create('course-stage-solution', { - changedFiles: [ - { - diff: '@@ -1,10 +1,11 @@\n import socket # noqa: F401\n\n\n def main():\n server_socket = socket.create_server(("localhost", 6379), reuse_port=True)\n- server_socket.accept() # wait for client\n+ connection, _ = server_socket.accept()\n+ connection.sendall(b"+PONG\\r\\n")\n\n\n if __name__ == "__main__":\n main()\n', - filename: 'app/main.py', - }, - ], + this.server.create('course-stage-solution', { + changedFiles: loadFixture('redis-stage-2-python-solution.diff'),tests/acceptance/course-page/complete-second-stage-test.js (2)
80-83: Consider making the test message assertion more maintainableHardcoding the exact message text in tests can make them brittle to copy changes. Consider extracting this message to a constants file or using a more flexible assertion that checks for key phrases instead of the exact text.
Example approach:
-assert.contains(coursePage.secondStageTutorialCard.steps[1].instructions, 'To run tests, make changes to your code'); +const EXPECTED_KEYWORDS = ['run tests', 'make changes']; +const instructions = coursePage.secondStageTutorialCard.steps[1].instructions; +EXPECTED_KEYWORDS.forEach(keyword => + assert.contains(instructions, keyword, `Instructions should contain "${keyword}"`));
Line range hint
1-150: Consider reducing setup duplication between testsBoth test cases share similar setup code for creating the test scenario, signing in, and setting up the repository. Consider extracting this common setup into a helper function or hooks to improve maintainability.
Example approach:
// Helper function to set up common test state async function setupTestRepository(server, { clientType = 'system' } = {}) { const currentUser = server.schema.users.first(); const python = server.schema.languages.findBy({ name: 'Python' }); const course = server.schema.courses.findBy({ slug: 'dummy' }); course.update({ releaseStatus: 'live' }); return server.create('repository', 'withFirstStageCompleted', { course: course, language: python, user: currentUser, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
app/components/course-page/course-stage-step/embedded-language-guide-card.hbs(1 hunks)tests/acceptance/course-page/complete-first-stage-test.js(1 hunks)tests/acceptance/course-page/complete-second-stage-test.js(3 hunks)tests/acceptance/course-page/complete-stage-without-changes-test.js(0 hunks)tests/acceptance/course-page/course-stage-solutions-test.js(0 hunks)tests/acceptance/course-page/language-guides-test.js(2 hunks)tests/pages/components/course-page/course-stage-step/second-stage-tutorial-card.ts(0 hunks)tests/pages/course-page.js(1 hunks)
💤 Files with no reviewable changes (3)
- tests/acceptance/course-page/complete-stage-without-changes-test.js
- tests/acceptance/course-page/course-stage-solutions-test.js
- tests/pages/components/course-page/course-stage-step/second-stage-tutorial-card.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/components/course-page/course-stage-step/embedded-language-guide-card.hbs
- tests/acceptance/course-page/complete-first-stage-test.js
- tests/pages/course-page.js
🔇 Additional comments (4)
tests/acceptance/course-page/language-guides-test.js (2)
37-42: LGTM! Redundant repository creation has been removed.
The test now correctly creates only one repository, addressing the previous review comment about redundant repository creation.
49-59: Consider adding test cases for error scenarios.
While the happy path is well tested, consider adding test cases for:
- What happens if the language guide content is empty?
- How does the UI handle very long explanations?
- What if the solution reveal fails?
This will ensure the component handles edge cases gracefully.
Let's check if these scenarios are covered elsewhere:
tests/acceptance/course-page/complete-second-stage-test.js (2)
72-79: LGTM: Step expansion state assertions are comprehensive
The test properly validates the interaction between steps, ensuring correct expansion/collapse behavior after user interaction.
41-41: Verify intended behavior change for first step expansion
The test now expects the first step to be expanded by default, which appears to be a behavioral change from the previous implementation. This aligns with the removal of ReadInstructionsStep mentioned in the architectural changes.
Let's verify this is the intended behavior across all related tests:
Also applies to: 55-55
✅ Verification successful
Behavior of first step expansion is consistently tested
The test assertions for the first step's expansion state are consistent throughout the test file. The first step is expected to be expanded in most scenarios except when specific conditions are met (like when another step is active). This behavior appears to be well-defined and properly tested with both positive and negative cases, confirming this is indeed the intended behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other tests that might need similar updates
rg -A 2 "secondStageTutorialCard.steps\[0\]" "tests/" | grep -E "isExpanded|expanded"
Length of output: 1829
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
tests/acceptance/course-page/language-guides-test.js (1)
27-35: Consider extracting the diff content to a fixture fileThe inline diff makes the test setup harder to maintain. Consider moving it to a separate fixture file for better readability and maintainability.
Example refactor:
- this.server.create('course-stage-solution', { - changedFiles: [ - { - diff: '@@ -1,10 +1,11 @@\n import socket...', - filename: 'app/main.py', - }, - ], +this.server.create('course-stage-solution', { + changedFiles: [{ + diff: loadFixture('redis/stage-2/python-solution.diff'), + filename: 'app/main.py' + }],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs(2 hunks)app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.ts(3 hunks)tests/acceptance/course-page/language-guides-test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.ts
[warning] 30-30: app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.ts#L30
Added line #L30 was not covered by tests
🔇 Additional comments (3)
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.ts (2)
4-4: LGTM!
The import statement for CourseStageLanguageGuideModel is properly typed and follows the project's naming conventions.
15-15: LGTM! Good improvement in the component interface
The change from a boolean flag to an optional model property is a better design choice. It makes the component's dependencies more explicit and provides direct access to the language guide data when needed.
tests/acceptance/course-page/language-guides-test.js (1)
37-42: LGTM! Repository setup looks good
The repository creation is properly configured with the necessary attributes and the previous issue of redundant repository creation has been addressed.
...mponents/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/acceptance/course-page/course-stage-solutions-test.js (1)
44-62: Consider adding error handling for navigation steps.While the test is well-structured, consider adding error handling or assertions after navigation steps to ensure the page has loaded correctly before proceeding with the test.
Example:
// Add after navigation assert.ok(coursePage.isLoaded, 'Course page should be loaded');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs(2 hunks)tests/acceptance/course-page/course-stage-solutions-test.js(1 hunks)tests/pages/components/course-page/course-stage-step/second-stage-tutorial-card.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs
- tests/pages/components/course-page/course-stage-step/second-stage-tutorial-card.ts
🧰 Additional context used
🪛 eslint
tests/acceptance/course-page/course-stage-solutions-test.js
[error] 63-63: Do not commit pauseTest()
(ember/no-pause-test)
🔇 Additional comments (1)
tests/acceptance/course-page/course-stage-solutions-test.js (1)
44-62: LGTM! Well-structured test with comprehensive state verification.
The test effectively verifies all solution visibility states with clear assertions and appropriate Percy snapshots.
andy1li
force-pushed
the
stage-2-prototype
branch
from
07b6640 to
497f6d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs (2)
Line range hint
18-31: Consider extracting file diff rendering into a separate component.The file diff rendering logic could be moved to a dedicated component to improve maintainability and reusability.
- <div class="grid gap-3 mb-3"> - {{#each this.solution.changedFiles as |changedFile|}} - {{! Extra if condition convinces typescript that solution isn't null }} - {{#if this.solution}} - <FileDiffCard - @filename={{changedFile.filename}} - @code={{changedFile.diff}} - @language={{this.solution.language.slug}} - data-test-file-diff-card - /> - {{/if}} - {{/each}} - </div> + <SolutionFileDiffs + @files={{this.solution.changedFiles}} + @language={{this.solution.language.slug}} + />
32-70: Consolidate button components and dark mode styles.The hide/reveal solution buttons share similar patterns and styles that could be consolidated.
Consider creating a shared
SolutionToggleButtoncomponent that accepts an@isHiddenargument to determine the icon and text to display. This would reduce code duplication and make it easier to maintain consistent styling.Also, the dark mode classes could be consolidated into CSS classes or Tailwind components to reduce repetition and improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs(2 hunks)app/models/course-stage.ts(1 hunks)tests/acceptance/course-page/course-stage-solutions-test.js(1 hunks)tests/pages/components/course-page/course-stage-step/second-stage-tutorial-card.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/models/course-stage.ts
- tests/acceptance/course-page/course-stage-solutions-test.js
- tests/pages/components/course-page/course-stage-step/second-stage-tutorial-card.ts
🔇 Additional comments (2)
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs (2)
Line range hint 1-73: Implementation aligns with Stage 2 prototype requirements.
The changes successfully implement the new tutorial card behavior with proper test coverage (data-test attributes) and visual considerations (Percy-compatible styling).
1-9: Consider standardizing course instructions across all courses.
The conditional rendering of shortInstructionsMarkdown only for the "shell" course could lead to inconsistent user experience. Consider extending this feature to other courses or implementing a more generic approach.
Let's verify if other courses could benefit from similar instructions:
...ponents/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs
Show resolved
Hide resolved
andy1li
force-pushed
the
stage-2-prototype
branch
from
497f6d2 to
d24f49d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests/acceptance/course-page/language-guides-test.js (2)
27-35: Consider moving the solution diff to a test fixtureThe hardcoded diff makes the test brittle and harder to maintain. Consider extracting it to a fixture file for better maintainability and reusability.
Create a new fixture file (e.g.,
tests/fixtures/redis-python-stage-2.js):export const redisPythonStage2Solution = { changedFiles: [ { filename: 'app/main.py', diff: '@@ -1,10 +1,11 @@\n import socket # noqa: F401\n\n\n def main():\n server_socket = socket.create_server(("localhost", 6379), reuse_port=True)\n- server_socket.accept() # wait for client\n+ connection, _ = server_socket.accept()\n+ connection.sendall(b"+PONG\\r\\n")\n\n\n if __name__ == "__main__":\n main()\n' } ] };Then import and use it in the test:
+ import { redisPythonStage2Solution } from '../fixtures/redis-python-stage-2'; this.server.create('course-stage-solution', { - changedFiles: [ - { - diff: '...', - filename: 'app/main.py', - }, - ], + ...redisPythonStage2Solution, courseStage: redis.stages.models.sortBy('position')[1], language: python, });
Line range hint
1-1: Consider renaming the test file to match its purposeThe test file name
language-guides-test.jsdoesn't accurately reflect its current purpose, which now focuses on solution reveal functionality with language guides.Consider renaming to
course-stage-solution-test.jsor moving these tests to a more appropriate test file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs(2 hunks)app/models/course-stage.ts(1 hunks)tests/acceptance/course-page/course-stage-solutions-test.js(1 hunks)tests/acceptance/course-page/language-guides-test.js(1 hunks)tests/pages/components/course-page/course-stage-step/second-stage-tutorial-card.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs
- app/models/course-stage.ts
- tests/acceptance/course-page/course-stage-solutions-test.js
- tests/pages/components/course-page/course-stage-step/second-stage-tutorial-card.ts
🔇 Additional comments (2)
tests/acceptance/course-page/language-guides-test.js (2)
38-42: LGTM! Previous redundancy issue resolved.
The redundant repository creation identified in the previous review has been addressed.
47-49:
Add missing assertions for solution visibility
The test only verifies the language guide content but doesn't assert the solution visibility state after clicking the blurred overlay.
Add assertions to verify the complete interaction flow:
await coursePage.secondStageTutorialCard.clickOnSolutionBlurredOverlay();
+assert.true(coursePage.secondStageTutorialCard.isSolutionVisible, 'Solution should be visible after reveal');
+assert.true(coursePage.languageGuideCard.isVisible, 'Language guide should be visible');
assert.strictEqual(coursePage.languageGuideCard.text, 'In this stage, blah blah…', 'Language guide card displays the correct content');…tage tutorial card.
andy1li
force-pushed
the
stage-2-prototype
branch
from
2b713ed to
fce4b86
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app/models/course-stage.ts (1)
145-157: Consider making the instructions configurable and course-specific.The hardcoded instructions might limit flexibility and reusability. Consider the following improvements:
- Move the instructions template to a configuration file or database.
- Add a conditional check to ensure these instructions are only shown for shell-related courses.
- Consider using TypeScript template literal types to ensure markdown formatting consistency.
Here's a suggested refactor:
get shortInstructionsMarkdown() { + if (this.course.slug !== 'shell') { + return null; + } + + return this.store.peekRecord('course-stage-instructions', this.slug)?.shortInstructions ?? ` In this stage, you'll implement support for handling missing commands in your shell. Example: \`\`\`bash $ invalid_command invalid_command: not found \`\`\` `; }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 146-146: app/models/course-stage.ts#L146
Added line #L146 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs(2 hunks)app/models/course-stage.ts(1 hunks)tests/acceptance/course-page/course-stage-solutions-test.js(1 hunks)tests/acceptance/course-page/language-guides-test.js(1 hunks)tests/pages/components/course-page/course-stage-step/second-stage-tutorial-card.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs
- tests/acceptance/course-page/course-stage-solutions-test.js
- tests/pages/components/course-page/course-stage-step/second-stage-tutorial-card.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/models/course-stage.ts
[warning] 146-146: app/models/course-stage.ts#L146
Added line #L146 was not covered by tests
🔇 Additional comments (3)
tests/acceptance/course-page/language-guides-test.js (3)
38-42: LGTM! Redundant repository creation removed
The code now creates only one repository, addressing the previous review comment about redundant repository creation.
47-47: 🛠️ Refactor suggestion
Add assertions to verify solution reveal state
The test should verify the state changes after revealing the solution.
Add assertions to verify:
- Initial state (solution hidden)
- State after reveal (solution visible)
- Solution content visibility
+ assert.false(coursePage.secondStageTutorialCard.isSolutionVisible, 'Solution should be hidden initially');
await coursePage.secondStageTutorialCard.clickOnSolutionBlurredOverlay();
+ assert.true(coursePage.secondStageTutorialCard.isSolutionVisible, 'Solution should be visible after reveal');49-49: 🛠️ Refactor suggestion
Add assertions for solution content
The test only verifies the language guide content but should also verify the solution content is displayed correctly.
Add assertions to verify the solution content:
assert.strictEqual(coursePage.languageGuideCard.text, 'In this stage, blah blah…', 'Language guide card displays the correct content');
+ assert.true(coursePage.secondStageTutorialCard.solutionContent.includes('socket.create_server'), 'Solution content should be visible');
+ assert.true(coursePage.secondStageTutorialCard.solutionContent.includes('connection.sendall'), 'Solution should show the PING implementation');Likely invalid or redundant comment.
andy1li
force-pushed
the
stage-2-prototype
branch
from
fce4b86 to
f4db679
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/models/course-stage.ts (1)
145-156: Improve type safety and documentationConsider the following improvements:
- Extract the markdown template to a constant for better maintainability
- Add TSDoc comments explaining the purpose and usage of this getter
Here's a suggested implementation:
+ /** Template for shell command error handling instructions */ + const SHELL_ERROR_INSTRUCTIONS = ` + In this stage, you'll implement support for handling invalid commands in your shell. + + Example: + + \`\`\` + $ invalid_command + invalid_command: not found + \`\`\` + ` as const; + + /** + * Returns markdown instructions for implementing shell command error handling. + * @throws {Error} If accessed for non-shell courses + */ get shortInstructionsMarkdown() { if (this.course.slug !== 'shell') { throw new Error('shortInstructionsMarkdown is only available for shell courses'); } - return ` -In this stage, you'll implement support for handling invalid commands in your shell. - -Example: - -\`\`\` -$ invalid_command -invalid_command: not found -\`\`\` -`; + return SHELL_ERROR_INSTRUCTIONS; }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 146-146: app/models/course-stage.ts#L146
Added line #L146 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs(3 hunks)app/models/course-stage.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/course-page/course-stage-step/second-stage-tutorial-card/implement-solution-step.hbs
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/models/course-stage.ts
[warning] 146-146: app/models/course-stage.ts#L146
Added line #L146 was not covered by tests
🔇 Additional comments (1)
app/models/course-stage.ts (1)
145-156: Add test coverage for the shortInstructionsMarkdown getter
The new getter lacks test coverage as indicated by the static analysis.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 146-146: app/models/course-stage.ts#L146
Added line #L146 was not covered by tests
Andy Checklist:
[percy]in the message to trigger)Summary by CodeRabbit
Release Notes
New Features
shortInstructionsMarkdownmethod for better guidance.ImplementSolutionStepcomponent, focusing on clearer visibility of solutions.ExpandableStepListComponent.Bug Fixes
Documentation
Chores