feat: add Rust language support and concept group handling #2672
Conversation
WalkthroughThis pull request introduces enhancements to data retrieval and UI presentation in track-related functionality. It adds new type imports and computed properties in both the Changes
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🪧 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 (
|
Test Results 1 files ±0 1 suites ±0 6m 48s ⏱️ +10s For more details on these errors, see this check. Results for commit 58a5138. ± Comparison against base commit e5c1f7d. This pull request skips 4 tests.♻️ This comment has been updated with latest results. |
❌ 6 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/models/language.ts (1)
252-258: Consider making the concept group slug configurable.The hard-coded 'rust-primer' slug might make it difficult to change the concept group identifier in the future. Consider moving this to a configuration object alongside other language-specific settings.
get trackPrimerConceptGroupSlug() { - if (this.isRust) { - return 'rust-primer'; - } else { - return null; - } + const CONCEPT_GROUP_SLUGS = { + rust: 'rust-primer' + }; + return CONCEPT_GROUP_SLUGS[this.slug] || null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/track.ts(2 hunks)app/models/language.ts(2 hunks)app/routes/track.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (2)
app/models/language.ts (1)
145-147: LGTM!The
isRustgetter follows the established pattern used for other language checks in the codebase.app/controllers/track.ts (1)
31-37: LGTM!The implementation correctly uses
peekAllto retrieve the concept group, as the data is already loaded by the route. The type safety is well-maintained with proper null handling.
app/routes/track.ts
Outdated
| if (language.trackPrimerConceptGroupSlug) { | ||
| const conceptGroup = await this.store.queryRecord('concept-group', { slug: language.trackPrimerConceptGroupSlug, include: 'author' }); | ||
|
|
||
| if (conceptGroup) { | ||
| await this.store.findAll('concept', { include: 'author,questions' }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize concept loading and add error handling.
Two suggestions for improvement:
- Consider using
queryinstead offindAllto load only the concepts related to this concept group. - Add error handling for the queries.
if (language.trackPrimerConceptGroupSlug) {
- const conceptGroup = await this.store.queryRecord('concept-group', { slug: language.trackPrimerConceptGroupSlug, include: 'author' });
+ try {
+ const conceptGroup = await this.store.queryRecord('concept-group', {
+ slug: language.trackPrimerConceptGroupSlug,
+ include: 'author'
+ });
+ if (conceptGroup) {
+ await this.store.query('concept', {
+ filter: {
+ conceptGroupId: conceptGroup.id
+ },
+ include: 'author,questions'
+ });
+ }
+ } catch (error) {
+ console.error('Failed to load concept group:', error);
+ }
- if (conceptGroup) {
- await this.store.findAll('concept', { include: 'author,questions' });
- }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (language.trackPrimerConceptGroupSlug) { | |
| const conceptGroup = await this.store.queryRecord('concept-group', { slug: language.trackPrimerConceptGroupSlug, include: 'author' }); | |
| if (conceptGroup) { | |
| await this.store.findAll('concept', { include: 'author,questions' }); | |
| } | |
| } | |
| if (language.trackPrimerConceptGroupSlug) { | |
| try { | |
| const conceptGroup = await this.store.queryRecord('concept-group', { | |
| slug: language.trackPrimerConceptGroupSlug, | |
| include: 'author' | |
| }); | |
| if (conceptGroup) { | |
| await this.store.query('concept', { | |
| filter: { | |
| conceptGroupId: conceptGroup.id | |
| }, | |
| include: 'author,questions' | |
| }); | |
| } | |
| } catch (error) { | |
| console.error('Failed to load concept group:', error); | |
| } | |
| } |
Bundle ReportChanges will increase total bundle size by 5.12kB (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: client-array-pushAssets Changed:
Files in
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/components/track-page/course-card-list.ts (1)
24-39: Consider refactoringcoursesWithProgressfor better readability.The getter's filtering logic could be simplified for better maintainability.
Consider this refactor:
get coursesWithProgress() { return this.args.courses.map((course) => { - const repositoryWithMostProgress = this.authenticator.currentUser - ? this.authenticator.currentUser.repositories - .filterBy('language', this.args.language) - .filterBy('course', course) - .filterBy('firstSubmissionCreated') - .sortBy('completedStages.length', 'lastSubmissionAt').lastObject - : null; + const repositoryWithMostProgress = this.getMostProgressedRepository(course); return { repositoryWithMostProgress: repositoryWithMostProgress || undefined, course: course, }; }); } + + private getMostProgressedRepository(course: CourseModel) { + if (!this.authenticator.currentUser) { + return null; + } + + return this.authenticator.currentUser.repositories + .filterBy('language', this.args.language) + .filterBy('course', course) + .filterBy('firstSubmissionCreated') + .sortBy('completedStages.length', 'lastSubmissionAt') + .lastObject; + }app/templates/join-track.hbs (1)
40-40: Reminder: Address the TODO comment about track testimonials.The TODO comment suggests showing testimonials specific to tracks instead of courses.
Would you like me to help implement track-specific testimonials or create an issue to track this enhancement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/track-page/course-card-list.ts(2 hunks)app/templates/join-track.hbs(1 hunks)app/templates/track.hbs(1 hunks)
🔇 Additional comments (3)
app/templates/track.hbs (1)
13-13: LGTM! Component replacement maintains functionality.The component replacement from
TrackPage::IntroductionAndCoursestoTrackPage::CourseCardListis consistent across templates and maintains the same parameters and styling.app/components/track-page/course-card-list.ts (1)
18-18: LGTM! Component renaming is consistent.The class name change and component registry update maintain consistency with the broader refactoring effort.
Also applies to: 44-44
app/templates/join-track.hbs (1)
27-27: LGTM! Component replacement maintains consistency.The component replacement from
TrackPage::IntroductionAndCoursestoTrackPage::CourseCardListis consistent with other templates and maintains the same parameters and styling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
app/components/track-page/primer-concept-group-card/index.hbs (1)
5-5: Consider making the text content dynamic.The title "Rust Primer" and the description are hardcoded. To support future language primers, consider making these dynamic based on the
@languageargument.- @title="Rust Primer" + @title="{{@language.name}} Primer"- New to Rust? Start with our interactive concepts that cover the basics. + New to {{@language.name}}? Start with our interactive concepts that cover the basics.Also applies to: 17-17
app/components/track-page/primer-concept-group-card/concept-list.ts (2)
18-28: Consider handling missing concepts more gracefully.The
sortedConceptsgetter silently skips concepts that aren't found in the store. This could lead to confusion if concepts are missing unexpectedly.Consider adding error logging or handling:
get sortedConcepts() { return this.args.conceptGroup.conceptSlugs.reduce((acc, slug) => { const concept = this.store.peekAll('concept').find((concept) => concept.slug === slug); if (concept) { acc.push(concept); + } else { + console.warn(`Concept with slug "${slug}" not found in store`); } return acc; }, [] as ConceptModel[]); }
19-21: Consider using findBy for better performance.Using
peekAll().find()requires iterating through all concepts to find a match.Consider using
peekRecordinstead:-const concept = this.store.peekAll('concept').find((concept) => concept.slug === slug); +const concept = this.store.peekRecord('concept', slug);app/components/track-page/primer-concept-group-card/concept-list-item.hbs (2)
12-13: Address the TODO comment about heroicons qualification.The comment indicates a need to qualify all heroicons with solid/outline variants.
Would you like me to help implement the heroicon qualification for better consistency?
23-30: Consider improving accessibility for reading time.The reading time information could benefit from better accessibility.
Add ARIA attributes for screen readers:
-<div class="items-center gap-1 pl-4 hidden sm:flex flex-shrink-0"> +<div class="items-center gap-1 pl-4 hidden sm:flex flex-shrink-0" role="complementary" aria-label="Estimated reading time"> {{svg-jar "clock" class="w-4 fill-current text-gray-300 dark:text-gray-700"}} - <span class="text-xs text-gray-400 dark:text-gray-600"> + <span class="text-xs text-gray-400 dark:text-gray-600" aria-label="{{@concept.estimatedReadingTimeInMinutes}} minutes to read"> {{@concept.estimatedReadingTimeInMinutes}} mins </span> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/assets/images/heroicons/outline/book-open-outline.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
app/components/track-page/card.hbs(1 hunks)app/components/track-page/course-card.hbs(2 hunks)app/components/track-page/primer-concept-group-card/concept-list-item.hbs(1 hunks)app/components/track-page/primer-concept-group-card/concept-list-item.ts(1 hunks)app/components/track-page/primer-concept-group-card/concept-list.hbs(1 hunks)app/components/track-page/primer-concept-group-card/concept-list.ts(1 hunks)app/components/track-page/primer-concept-group-card/index.hbs(1 hunks)app/components/track-page/primer-concept-group-card/index.ts(1 hunks)app/templates/track.hbs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/components/track-page/card.hbs
🚧 Files skipped from review as they are similar to previous changes (1)
- app/templates/track.hbs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (8)
app/components/track-page/course-card.hbs (2)
17-17: LGTM! Improved vertical spacing.The margin reduction from
mb-6tomb-4creates a more compact and modern layout while maintaining readability.
56-56: LGTM! Improved spacing consistency.The margin and padding changes align with the previous segment, ensuring consistent spacing throughout the component. The removal of
md:pr-12standardizes the padding across screen sizes.app/components/track-page/primer-concept-group-card/concept-list.hbs (1)
1-3: Verify if@isCompleteshould be hardcoded tofalse.The template correctly iterates over
sortedConcepts, but the@isCompleteattribute is hardcoded tofalse. This might prevent showing completed concepts.Should this value be dynamic based on the user's progress? Consider passing a computed property or a service value instead.
app/components/track-page/primer-concept-group-card/index.ts (1)
1-21: LGTM! Well-structured TypeScript component.The component is well-typed with proper interfaces and Glint registry integration. The implementation follows TypeScript and Ember best practices.
app/components/track-page/primer-concept-group-card/concept-list.ts (2)
7-13: LGTM! Well-defined TypeScript interface.The
Signatureinterface properly defines the component's element type and required arguments.
31-35: LGTM! Proper Glint registry declaration.The component is correctly registered in the Glint environment for type safety in templates.
app/components/track-page/primer-concept-group-card/concept-list-item.hbs (2)
1-7: LGTM! Well-structured LinkTo component.The LinkTo component is properly configured with route, models, and styling classes. The data-test attribute is helpful for testing.
8-18: LGTM! Clean and semantic layout structure.The flex layout with proper spacing and icon alignment creates a clear visual hierarchy.
| } | ||
|
|
||
| export default class ConceptListItemComponent extends Component<Signature> { | ||
| @service declare authenticator: AuthenticatorService; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused service injection.
The authenticator service is injected but not used in the component. If it's not needed, consider removing it.
- @service declare authenticator: AuthenticatorService;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @service declare authenticator: AuthenticatorService; |
rohitpaulk
force-pushed
the
add-track-primer-concept-group
branch
from
d4b4cfe to
ae5b823
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/components/track-page/primer-concept-group-card/index.hbs (1)
16-18: Remove or uncomment the commented code.The commented code appears to be the description text that matches the PR objectives. Either remove it if it's no longer needed or uncomment it if it's meant to be displayed.
app/templates/track.hbs (1)
21-26: Extract repeated prose sections into a component.The prose sections for basics and projects follow a similar structure. Consider extracting them into a reusable component to improve maintainability.
Create a new component
TrackPage::ProseSectionand use it like this:Also applies to: 37-43
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/assets/images/heroicons/outline/book-open-outline.svgis excluded by!**/*.svg
📒 Files selected for processing (14)
app/components/track-page/card.hbs(1 hunks)app/components/track-page/course-card-list.ts(2 hunks)app/components/track-page/course-card.hbs(2 hunks)app/components/track-page/primer-concept-group-card/concept-list-item.hbs(1 hunks)app/components/track-page/primer-concept-group-card/concept-list-item.ts(1 hunks)app/components/track-page/primer-concept-group-card/concept-list.hbs(1 hunks)app/components/track-page/primer-concept-group-card/concept-list.ts(1 hunks)app/components/track-page/primer-concept-group-card/index.hbs(1 hunks)app/components/track-page/primer-concept-group-card/index.ts(1 hunks)app/controllers/track.ts(2 hunks)app/models/language.ts(2 hunks)app/routes/track.ts(1 hunks)app/templates/join-track.hbs(1 hunks)app/templates/track.hbs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- app/components/track-page/card.hbs
- app/routes/track.ts
- app/components/track-page/primer-concept-group-card/concept-list-item.hbs
- app/components/track-page/primer-concept-group-card/concept-list.hbs
- app/components/track-page/course-card.hbs
- app/components/track-page/primer-concept-group-card/concept-list.ts
- app/models/language.ts
- app/components/track-page/course-card-list.ts
- app/components/track-page/primer-concept-group-card/concept-list-item.ts
- app/controllers/track.ts
- app/templates/join-track.hbs
- app/components/track-page/primer-concept-group-card/index.ts
🧰 Additional context used
🪛 GitHub Actions: Test
app/components/track-page/primer-concept-group-card/index.hbs
[warning] 1-1: Code style issues found in the above file. Run Prettier with --write to fix.
🔇 Additional comments (3)
app/components/track-page/primer-concept-group-card/index.hbs (1)
1-22:❓ Verification inconclusive
Fix code style issues.
The pipeline reports code style issues. Run Prettier with the --write flag to fix formatting.
🏁 Script executed:
#!/bin/bash # Description: Fix code style issues using Prettier. # Install prettier if not available npm install --save-dev prettier # Run prettier npx prettier --write "app/components/track-page/primer-concept-group-card/index.hbs"Length of output: 9388
Prettier Formatting Compliance – Manual Verification Needed
The Prettier command was executed on
app/components/track-page/primer-concept-group-card/index.hbswithout any reported formatting changes. This generally indicates that the file is already compliant with our code style guidelines. However, since the script output mainly included npm warnings unrelated to Prettier’s operation, please verify manually that no code style issues remain.
- Location:
app/components/track-page/primer-concept-group-card/index.hbs- Command Run:
npm install --save-dev prettier npx prettier --write "app/components/track-page/primer-concept-group-card/index.hbs"- Action Required: Manually inspect the file to ensure it adheres to our formatting standards.
🧰 Tools
🪛 GitHub Actions: Test
[warning] 1-1: Code style issues found in the above file. Run Prettier with --write to fix.
app/templates/track.hbs (2)
15-18: Verify color contrast for accessibility.The text colors
text-gray-800andtext-gray-200in dark mode need to be verified for sufficient contrast ratio with their backgrounds to ensure accessibility.#!/usr/bin/env python3 import colorsys import sys def hex_to_rgb(hex_color): hex_color = hex_color.lstrip('#') return tuple(int(hex_color[i:i+2], 16) for i in (0, 2, 4)) def relative_luminance(rgb): srgb = [x/255 for x in rgb] srgb = [x/12.92 if x <= 0.03928 else ((x+0.055)/1.055)**2.4 for x in srgb] return 0.2126 * srgb[0] + 0.7152 * srgb[1] + 0.0722 * srgb[2] def contrast_ratio(color1, color2): l1 = relative_luminance(hex_to_rgb(color1)) l2 = relative_luminance(hex_to_rgb(color2)) return (max(l1, l2) + 0.05) / (min(l1, l2) + 0.05) # Check contrast ratios text_colors = { 'light': {'text': '#1F2937', 'bg': '#FFFFFF'}, # gray-800 on white 'dark': {'text': '#E5E7EB', 'bg': '#111827'} # gray-200 on gray-850 } for mode, colors in text_colors.items(): ratio = contrast_ratio(colors['text'], colors['bg']) print(f"{mode.title()} mode contrast ratio: {ratio:.2f}") if ratio < 4.5: # WCAG AA standard for normal text print(f"Warning: {mode} mode contrast ratio is below WCAG AA standard")Also applies to: 31-34
13-44: LGTM! The conditional rendering looks good.The conditional block for
trackPrimerConceptGroupis well-structured and provides a clear separation between basics and projects sections.
| @isComplete={{false}} | ||
| @logoImageAltText={{@language.name}} | ||
| @logoImageUrl={{@language.colorLogoUrl}} | ||
| @title="Rust Primer" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the title dynamic based on the language.
The title "Rust Primer" is hard-coded, which limits the component's reusability for other languages. Consider using the language name from the @language parameter.
- @title="Rust Primer"
+ @title="{{@language.name}} Primer"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @title="Rust Primer" | |
| @title="{{@language.name}} Primer" |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/components/track-page/primer-concept-group-card/concept-list-item.hbs (1)
33-33: Consider enhancing tooltip accessibility.While the tooltip implementation is functional, consider adding ARIA attributes and ensuring the markdown content is properly sanitized for accessibility.
Consider these improvements:
- <EmberTooltip @text={{@concept.descriptionMarkdown}} /> + <EmberTooltip + @text={{@concept.descriptionMarkdown}} + @ariaLabel="Concept description" + @role="tooltip" + />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/components/track-page/primer-concept-group-card/concept-list-item.hbs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (3)
app/components/track-page/primer-concept-group-card/concept-list-item.hbs (3)
1-7: LGTM! Well-structured navigation component.The LinkTo component is properly implemented with appropriate hover states for both light and dark modes, test attributes, and extensibility through attribute spreading.
20-31: LGTM! Clean conditional rendering of completion status.The implementation elegantly handles both completed and incomplete states, with appropriate visual feedback and user-friendly reading time display.
12-13:❓ Verification inconclusive
Address the TODO comment regarding heroicon qualification.
The comment suggests a need to qualify heroicons with solid/outline variants. This should be addressed to ensure consistent icon styling across the application.
Let's check if there's a consistent pattern for heroicon usage:
🏁 Script executed:
#!/bin/bash # Search for heroicon usage patterns rg -g '*.hbs' "svg-jar.*heroicon"Length of output: 35
Ensure all heroicons are explicitly qualified with either "solid" or "outline".
It appears that the only instance in this file—using
svg-jar "book-open-outline"—already follows the required naming pattern. However, the initial automatic search for heroicon usage (i.e. matching “svg-jar.*heroicon”) returned no results, so we couldn’t conclusively verify that similar patterns aren’t used elsewhere in the codebase. Please manually verify that every instance of an icon (invoked viasvg-jar) is named with a suffix of either-solidor-outline. If all are correctly qualified, consider removing the TODO comment; otherwise, update the non-compliant icon invocations accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/components/track-page/primer-concept-group-section/concept-list.hbs (1)
1-3: Consider making theisCompleteproperty dynamic.The
isCompleteproperty is currently hardcoded tofalse. Consider making it dynamic based on user progress to accurately reflect the completion status of each concept.app/components/track-page/primer-concept-group-section/concept-list-item.hbs (2)
12-14: Address the TODO comment about heroicons qualification.The comment indicates a need to specify whether the icon should use the solid or outline variant.
Would you like me to help implement the proper heroicon qualification? I can suggest the appropriate variant based on the design system.
33-34: Consider rendering Markdown content in the tooltip.The tooltip is currently displaying raw Markdown content which might not be readable.
Consider using a Markdown renderer component:
- <EmberTooltip @text={{@concept.descriptionMarkdown}} /> + <EmberTooltip> + <MarkdownRenderer @markdown={{@concept.descriptionMarkdown}} /> + </EmberTooltip>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/components/track-page/primer-concept-group-section/concept-list-item.hbs(1 hunks)app/components/track-page/primer-concept-group-section/concept-list-item.ts(1 hunks)app/components/track-page/primer-concept-group-section/concept-list.hbs(1 hunks)app/components/track-page/primer-concept-group-section/concept-list.ts(1 hunks)app/components/track-page/primer-concept-group-section/index.hbs(1 hunks)app/components/track-page/primer-concept-group-section/index.ts(1 hunks)app/templates/track.hbs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/templates/track.hbs
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/components/track-page/primer-concept-group-section/concept-list.ts
[warning] 20-21: app/components/track-page/primer-concept-group-section/concept-list.ts#L20-L21
Added lines #L20 - L21 were not covered by tests
[warning] 24-24: app/components/track-page/primer-concept-group-section/concept-list.ts#L24
Added line #L24 was not covered by tests
[warning] 27-27: app/components/track-page/primer-concept-group-section/concept-list.ts#L27
Added line #L27 was not covered by tests
🔇 Additional comments (8)
app/components/track-page/primer-concept-group-section/index.hbs (1)
1-21: LGTM! Well-structured template with semantic HTML.The template is well-organized with proper semantic HTML, consistent styling classes, and clear component hierarchy. The use of tooltips and pills enhances the user experience by providing additional context.
app/components/track-page/primer-concept-group-section/index.ts (1)
1-21: LGTM! Well-typed component with proper type safety.The component is well-structured with:
- Clear type definitions for props using TypeScript interfaces
- Proper Glint registry setup for type checking
- Correct extension of Glimmer component
app/components/track-page/primer-concept-group-section/concept-list-item.ts (1)
15-17:✅ Verification successful
Verify the usage of the authenticator service.
The
authenticatorservice is injected but doesn't appear to be used within the component. If it's not needed, consider removing it to keep the code clean.Run the following script to check if the service is used in the template:
🏁 Script executed:
#!/bin/bash # Description: Check if the authenticator service is used in the template. # Test: Search for authenticator usage in the template. rg -A 5 $'authenticator' app/components/track-page/primer-concept-group-section/concept-list-item.hbsLength of output: 100
Below is an additional verification step. It appears that the template check produced no output for any usage of the
authenticatorservice. To further confirm that the service isn’t used in the component’s TypeScript code, please run the following script:If that script also returns no results, it means the injected
authenticatorservice isn’t being referenced either in the template or in the component’s TypeScript. In that case, you should consider removing it to keep the code clean.Please verify the results of the above script and ensure that no usage of
this.authenticatorexists in the file.
🏁 Script executed:
#!/bin/bash # Description: Check if the authenticator service is used anywhere within the ConceptListItemComponent class # by searching for any occurrences of "this.authenticator" in the file. rg "this.authenticator" app/components/track-page/primer-concept-group-section/concept-list-item.tsLength of output: 99
Suggestion: Remove the Unused Authenticator Service Injection
The verification shows that the injected
authenticatorservice is not referenced in either the component’s TypeScript file (no occurrences ofthis.authenticator) or in its corresponding template. If the service isn’t planned for use, please consider removing it to keep the component clean and maintainable.app/components/track-page/primer-concept-group-section/concept-list.ts (3)
1-13: LGTM! Well-structured TypeScript interface and imports.The component signature is properly typed with appropriate imports.
15-17: LGTM! Clean component setup with proper service injection.The component is correctly defined with TypeScript and the store service is properly injected.
31-35: LGTM! Proper Glint registry declaration.The component is correctly registered in the Glint registry for template type checking.
app/components/track-page/primer-concept-group-section/concept-list-item.hbs (2)
1-7: LGTM! Well-structured LinkTo component with proper routing and styling.Good use of utility classes and includes data-test attribute for testing.
20-31: LGTM! Clear completion status indication with proper conditional rendering.Good use of icons and time estimation display.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/components/track-page/primer-concept-group-section/index.ts (1)
19-24: Consider adding a guard clause for the collapse action.The collapse action could be optimized to avoid unnecessary scheduling when already collapsed.
@action handleCollapseButtonClick() { + if (this.conceptListIsExpanded) { next(() => { this.conceptListIsExpanded = false; }); + } }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 22-23: app/components/track-page/primer-concept-group-section/index.ts#L22-L23
Added lines #L22 - L23 were not covered by testsapp/components/track-page/primer-concept-group-section/index.hbs (1)
24-40: Consider improving responsiveness of collapsed state.The fixed height (
h-60) for the collapsed state might not work well with varying content lengths. Also, the customvertical-maskclass should be documented.Consider these improvements:
- Use max-height instead of fixed height for better responsiveness
- Document the
vertical-maskclass implementation- <div class={{if this.conceptListIsExpanded "" "h-60 overflow-hidden"}}> + <div class={{if this.conceptListIsExpanded "" "max-h-60 overflow-hidden"}}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/components/track-page/primer-concept-group-section/index.hbs(1 hunks)app/components/track-page/primer-concept-group-section/index.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test
app/components/track-page/primer-concept-group-section/index.hbs
[error] 3-3: Interaction added to non-interactive element no-invalid-interactive
🪛 GitHub Check: codecov/patch
app/components/track-page/primer-concept-group-section/index.ts
[warning] 18-18: app/components/track-page/primer-concept-group-section/index.ts#L18
Added line #L18 was not covered by tests
[warning] 22-23: app/components/track-page/primer-concept-group-section/index.ts#L22-L23
Added lines #L22 - L23 were not covered by tests
[warning] 30-30: app/components/track-page/primer-concept-group-section/index.ts#L30
Added line #L30 was not covered by tests
🔇 Additional comments (5)
app/components/track-page/primer-concept-group-section/index.ts (3)
1-14: LGTM! Well-structured imports and type definitions.The imports are comprehensive and the TypeScript interface is well-defined with proper typing.
16-32: Add test coverage for interactive elements.While the implementation is correct, the static analysis indicates missing test coverage for the tracked property and action handlers. Please add tests to verify:
- Initial state of
conceptListIsExpanded- Collapse button functionality
- Section click behavior
Would you like me to help generate test cases for these scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 18-18: app/components/track-page/primer-concept-group-section/index.ts#L18
Added line #L18 was not covered by tests
[warning] 22-23: app/components/track-page/primer-concept-group-section/index.ts#L22-L23
Added lines #L22 - L23 were not covered by tests
[warning] 30-30: app/components/track-page/primer-concept-group-section/index.ts#L30
Added line #L30 was not covered by tests
34-38: LGTM! Proper Glint type registration.The module declaration correctly registers the component for type checking with an appropriate path-based name.
app/components/track-page/primer-concept-group-section/index.hbs (2)
6-15: Well-structured header with clear visual hierarchy!Good use of semantic markup, proper spacing, and helpful tooltip for the "FREE" pill.
17-22: Clean and responsive typography implementation!Good use of Tailwind's prose class for consistent typography and proper dark mode support.
| <div | ||
| class="relative group/concept-group-section {{unless this.conceptListIsExpanded 'cursor-pointer'}}" | ||
| {{on "click" this.handleSectionClick}} | ||
| ...attributes | ||
| > |
There was a problem hiding this comment.
Fix accessibility issue with click handler on div element.
The click handler on a non-interactive div element creates an accessibility issue. Users relying on screen readers may not recognize this as an interactive element.
Apply this diff to make the element properly interactive:
<div
class="relative group/concept-group-section {{unless this.conceptListIsExpanded 'cursor-pointer'}}"
+ role="button"
+ tabindex="0"
{{on "click" this.handleSectionClick}}
+ {{on "keypress" this.handleSectionClick}}
...attributes
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div | |
| class="relative group/concept-group-section {{unless this.conceptListIsExpanded 'cursor-pointer'}}" | |
| {{on "click" this.handleSectionClick}} | |
| ...attributes | |
| > | |
| <div | |
| class="relative group/concept-group-section {{unless this.conceptListIsExpanded 'cursor-pointer'}}" | |
| role="button" | |
| tabindex="0" | |
| {{on "click" this.handleSectionClick}} | |
| {{on "keypress" this.handleSectionClick}} | |
| ...attributes | |
| > |
🧰 Tools
🪛 GitHub Actions: Test
[error] 3-3: Interaction added to non-interactive element no-invalid-interactive
rohitpaulk
force-pushed
the
add-track-primer-concept-group
branch
from
29b0bdc to
18e6cb4
Compare
rohitpaulk
force-pushed
the
add-track-primer-concept-group
branch
from
6584f44 to
4b45116
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/acceptance/track-page/view-track-test.js (1)
38-41:⚠️ Potential issueReplace dummy assertion with meaningful checks.
The current test uses a dummy assertion that doesn't validate any actual behavior.
await visit('/tracks/rust'); - assert.strictEqual(1, 1); // dummy assertion + assert.ok(trackPage.primerConceptGroupSection.isVisible, 'Primer concept group section should be visible'); + assert.strictEqual(trackPage.primerConceptGroupSection.title, 'Rust Primer', 'Should show correct concept group title'); + assert.strictEqual(trackPage.primerConceptGroupSection.concepts.length, 2, 'Should show correct number of concepts'); await percySnapshot('Track (With Primer) - Anonymous User');
🧹 Nitpick comments (3)
app/components/track-page/primer-concept-group-section/index.ts (1)
26-31: Simplify state update by removing unnecessary next() wrapper.The
next()wrapper is unnecessary here since we're just updating a tracked property.@action handleCollapseButtonClick() { - next(() => { - this.conceptListIsExpanded = false; - }); + this.conceptListIsExpanded = false; }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 29-30: app/components/track-page/primer-concept-group-section/index.ts#L29-L30
Added lines #L29 - L30 were not covered by testsapp/models/language.ts (1)
148-150: Consider a more maintainable approach to language-specific logic.The current implementation adds individual getters for each language. As more languages are added, this could become harder to maintain.
Consider using a configuration object:
+ private static readonly LANGUAGE_CONFIG = { + rust: { + primerConceptGroupSlug: 'rust-primer' + }, + go: { + // ... existing go-specific config + } + } as const; - get isRust() { - return this.slug === 'rust'; - } - get trackPrimerConceptGroupSlug() { - if (this.isRust) { - return 'rust-primer'; - } else { - return null; - } - } + get isRust() { + return this.slug === 'rust'; + } + get trackPrimerConceptGroupSlug() { + return LanguageModel.LANGUAGE_CONFIG[this.slug as keyof typeof LanguageModel.LANGUAGE_CONFIG]?.primerConceptGroupSlug ?? null; + }Add tests for the new getters:
import { module, test } from 'qunit'; import { setupTest } from 'ember-qunit'; module('Unit | Model | language', function(hooks) { setupTest(hooks); test('isRust returns correct value', function(assert) { const store = this.owner.lookup('service:store'); let model = store.createRecord('language', { slug: 'rust' }); assert.true(model.isRust, 'Should be true for Rust'); model = store.createRecord('language', { slug: 'go' }); assert.false(model.isRust, 'Should be false for other languages'); }); test('trackPrimerConceptGroupSlug returns correct value', function(assert) { const store = this.owner.lookup('service:store'); let model = store.createRecord('language', { slug: 'rust' }); assert.strictEqual(model.trackPrimerConceptGroupSlug, 'rust-primer', 'Should return rust-primer for Rust'); model = store.createRecord('language', { slug: 'go' }); assert.strictEqual(model.trackPrimerConceptGroupSlug, null, 'Should return null for other languages'); }); });Also applies to: 255-261
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 149-149: app/models/language.ts#L149
Added line #L149 was not covered by teststests/acceptance/track-page/view-track-test.js (1)
151-183: Improve loading indicator test robustness.The loading indicator test could be more robust by:
- Adding timeout handling
- Verifying the loading state more thoroughly
- Adding error cases
test('visiting from catalog page has no loading page', async function (assert) { testScenario(this.server); createTrackLeaderboardEntries(this.server, 'rust', 'redis'); createConceptFromFixture(this.server, tcpOverview); createConceptFromFixture(this.server, networkProtocols); this.server.create('concept-group', { author: this.server.schema.users.first(), description_markdown: 'Dummy description', concept_slugs: ['tcp-overview', 'network-protocols', 'tcp-overview', 'network-protocols', 'tcp-overview', 'network-protocols'], slug: 'rust-primer', title: 'Rust Primer', }); let loadingIndicatorWasRendered = false; + const TIMEOUT = 5000; + const startTime = Date.now(); await catalogPage.visit(); catalogPage.clickOnTrack('Rust'); await waitUntil(() => { if (isSettled()) { return true; } else if (find('[data-test-loading]')) { loadingIndicatorWasRendered = true; } + + if (Date.now() - startTime > TIMEOUT) { + throw new Error('Test timed out waiting for page to settle'); + } }); assert.notOk(loadingIndicatorWasRendered, 'loading indicator was not rendered'); assert.true(trackPage.primerConceptGroupSection.isVisible, 'primer concept group section should be visible'); assert.strictEqual(trackPage.cards.length, 6, 'expected 6 track cards to be present (one per course)'); + + // Test error case + this.server.get('/api/tracks/rust', () => new Response(500)); + await catalogPage.clickOnTrack('Rust'); + assert.ok(find('[data-test-error]'), 'should show error state on API failure'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/assets/images/heroicons/outline/book-open-outline.svgis excluded by!**/*.svg
📒 Files selected for processing (24)
app/components/track-page/card.hbs(2 hunks)app/components/track-page/course-card-list.ts(2 hunks)app/components/track-page/course-card.hbs(2 hunks)app/components/track-page/header/index.hbs(1 hunks)app/components/track-page/primer-concept-group-section/concept-list-item.hbs(1 hunks)app/components/track-page/primer-concept-group-section/concept-list-item.ts(1 hunks)app/components/track-page/primer-concept-group-section/concept-list.hbs(1 hunks)app/components/track-page/primer-concept-group-section/concept-list.ts(1 hunks)app/components/track-page/primer-concept-group-section/index.hbs(1 hunks)app/components/track-page/primer-concept-group-section/index.ts(1 hunks)app/controllers/track.ts(1 hunks)app/models/language.ts(2 hunks)app/routes/catalog.ts(3 hunks)app/routes/track.ts(1 hunks)app/templates/join-track.hbs(1 hunks)app/templates/track.hbs(1 hunks)tests/acceptance/course-page/attempt-course-stage-test.js(1 hunks)tests/acceptance/course-page/resume-course-test.js(1 hunks)tests/acceptance/course-page/start-course-test.js(1 hunks)tests/acceptance/course-page/switch-repository-test.js(1 hunks)tests/acceptance/course-page/try-other-language-test.js(1 hunks)tests/acceptance/track-page/start-track-test.js(2 hunks)tests/acceptance/track-page/view-track-test.js(2 hunks)tests/pages/track-page.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (20)
- tests/pages/track-page.js
- app/templates/join-track.hbs
- tests/acceptance/course-page/resume-course-test.js
- app/routes/track.ts
- app/components/track-page/card.hbs
- app/components/track-page/course-card.hbs
- app/components/track-page/primer-concept-group-section/concept-list-item.hbs
- tests/acceptance/course-page/try-other-language-test.js
- app/components/track-page/primer-concept-group-section/concept-list.hbs
- app/components/track-page/header/index.hbs
- app/controllers/track.ts
- tests/acceptance/course-page/switch-repository-test.js
- app/routes/catalog.ts
- app/templates/track.hbs
- tests/acceptance/course-page/start-course-test.js
- app/components/track-page/primer-concept-group-section/index.hbs
- app/components/track-page/primer-concept-group-section/concept-list-item.ts
- app/components/track-page/course-card-list.ts
- tests/acceptance/course-page/attempt-course-stage-test.js
- tests/acceptance/track-page/start-track-test.js
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/components/track-page/primer-concept-group-section/index.ts
[warning] 25-25: app/components/track-page/primer-concept-group-section/index.ts#L25
Added line #L25 was not covered by tests
[warning] 29-30: app/components/track-page/primer-concept-group-section/index.ts#L29-L30
Added lines #L29 - L30 were not covered by tests
[warning] 37-37: app/components/track-page/primer-concept-group-section/index.ts#L37
Added line #L37 was not covered by tests
[warning] 41-41: app/components/track-page/primer-concept-group-section/index.ts#L41
Added line #L41 was not covered by tests
[warning] 43-43: app/components/track-page/primer-concept-group-section/index.ts#L43
Added line #L43 was not covered by tests
app/components/track-page/primer-concept-group-section/concept-list.ts
[warning] 20-21: app/components/track-page/primer-concept-group-section/concept-list.ts#L20-L21
Added lines #L20 - L21 were not covered by tests
[warning] 24-24: app/components/track-page/primer-concept-group-section/concept-list.ts#L24
Added line #L24 was not covered by tests
[warning] 27-27: app/components/track-page/primer-concept-group-section/concept-list.ts#L27
Added line #L27 was not covered by tests
app/models/language.ts
[warning] 149-149: app/models/language.ts#L149
Added line #L149 was not covered by tests
[warning] 257-257: app/models/language.ts#L257
Added line #L257 was not covered by tests
[warning] 259-259: app/models/language.ts#L259
Added line #L259 was not covered by tests
🔇 Additional comments (1)
app/components/track-page/primer-concept-group-section/index.ts (1)
33-44: Add test coverage for authentication and routing logic.The authentication and routing logic in
handleSectionClicklacks test coverage.Add these test cases:
import { module, test } from 'qunit'; import { setupTest } from 'ember-qunit'; module('Unit | Component | track-page/primer-concept-group-section', function(hooks) { setupTest(hooks); test('handleSectionClick expands list for authenticated users', function(assert) { const component = this.owner.lookup('component:track-page/primer-concept-group-section'); component.authenticator.isAuthenticated = true; component.conceptListIsExpanded = false; component.handleSectionClick(); assert.true(component.conceptListIsExpanded, 'List should be expanded'); }); test('handleSectionClick redirects unauthenticated users', function(assert) { const component = this.owner.lookup('component:track-page/primer-concept-group-section'); component.authenticator.isAuthenticated = false; component.args = { conceptGroup: { conceptSlugs: ['test-concept'] } }; component.handleSectionClick(); assert.true(component.router.transitionTo.calledWith('concept', 'test-concept'), 'Should redirect to concept page'); }); });🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 37-37: app/components/track-page/primer-concept-group-section/index.ts#L37
Added line #L37 was not covered by tests
[warning] 41-41: app/components/track-page/primer-concept-group-section/index.ts#L41
Added line #L41 was not covered by tests
[warning] 43-43: app/components/track-page/primer-concept-group-section/index.ts#L43
Added line #L43 was not covered by tests
| get sortedConcepts() { | ||
| return this.args.conceptGroup.conceptSlugs.reduce((acc, slug) => { | ||
| const concept = this.store.peekAll('concept').find((concept) => concept.slug === slug); | ||
|
|
||
| if (concept) { | ||
| acc.push(concept); | ||
| } | ||
|
|
||
| return acc; | ||
| }, [] as ConceptModel[]); | ||
| } |
There was a problem hiding this comment.
Use findAll() instead of peekAll() for reliable data fetching.
The current implementation has potential issues:
peekAll()only returns already loaded records, which could miss concepts if they haven't been loaded yet.- Missing error handling for cases where concepts are not found.
Consider this implementation:
get sortedConcepts() {
- return this.args.conceptGroup.conceptSlugs.reduce((acc, slug) => {
- const concept = this.store.peekAll('concept').find((concept) => concept.slug === slug);
-
- if (concept) {
- acc.push(concept);
- }
-
- return acc;
- }, [] as ConceptModel[]);
+ return Promise.all(
+ this.args.conceptGroup.conceptSlugs.map(async (slug) => {
+ try {
+ const concepts = await this.store.findAll('concept');
+ return concepts.find((concept) => concept.slug === slug);
+ } catch (error) {
+ console.error(`Failed to load concept ${slug}:`, error);
+ return null;
+ }
+ })
+ ).then((concepts) => concepts.filter((concept): concept is ConceptModel => concept !== null));
}The static analysis indicates missing test coverage. Let's add tests:
import { module, test } from 'qunit';
import { setupTest } from 'ember-qunit';
import { setupMirage } from 'ember-cli-mirage/test-support';
module('Unit | Component | track-page/primer-concept-group-section/concept-list', function(hooks) {
setupTest(hooks);
setupMirage(hooks);
test('sortedConcepts returns concepts in correct order', async function(assert) {
const store = this.owner.lookup('service:store');
const component = this.owner.lookup('component:track-page/primer-concept-group-section/concept-list');
// Setup test data
const conceptGroup = store.createRecord('concept-group', {
conceptSlugs: ['concept-1', 'concept-2']
});
// Create test concepts
store.createRecord('concept', { slug: 'concept-1', title: 'First Concept' });
store.createRecord('concept', { slug: 'concept-2', title: 'Second Concept' });
component.args = { conceptGroup };
const concepts = await component.sortedConcepts;
assert.strictEqual(concepts.length, 2, 'Returns all concepts');
assert.strictEqual(concepts[0].slug, 'concept-1', 'First concept is correct');
assert.strictEqual(concepts[1].slug, 'concept-2', 'Second concept is correct');
});
});🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 20-21: app/components/track-page/primer-concept-group-section/concept-list.ts#L20-L21
Added lines #L20 - L21 were not covered by tests
[warning] 24-24: app/components/track-page/primer-concept-group-section/concept-list.ts#L24
Added line #L24 was not covered by tests
[warning] 27-27: app/components/track-page/primer-concept-group-section/concept-list.ts#L27
Added line #L27 was not covered by tests
Adds support for the Rust language by introducing the `isRust` getter in the `LanguageModel`. Implements a new computed property `trackPrimerConceptGroupSlug` to return the appropriate slug for Rust. Updates the `TrackController` to include a method for retrieving the track primer concept group based on the language. Enhances the `TrackRoute` to query for the concept group when available, ensuring proper data loading for Rust tracks.
Create a new `TrackPageCourseCardList` component to encapsulate the logic for displaying course cards with progress. Move relevant functionality from `TrackPageIntroductionAndCourses` to the new component, improving code organization and reusability. Update template references to use the new component.
Implement the `ConceptList` and `ConceptListItem` components to display sorted concepts on the track page. Introduce a new SVG icon for visual representation. Update the card layout for better spacing and remove an unused SVG. These changes enhance the user interface and improve the organization of concept information.
Rearrange the clock icon in the concept list item to improve visual consistency. Update the text in the track template to better engage users by referencing the specific programming language, enhancing clarity and relevance.
Refactor the layout of the concept list item by removing the conditional class for the flex container, ensuring consistent alignment. Add a tooltip to display the concept's description, enhancing user experience by providing additional context.
Remove the old concept list components and create new ones for the primer concept group section. This change improves code organization and reusability by separating the concept list functionality into distinct components for group and section. The new components maintain the same logic for sorting concepts while enhancing readability and maintainability.
Adjust the layout of the track page template to enhance readability. Refactor the conditional rendering of the PrimerConceptGroupSection and improve the structure of the challenges section. Update the text to make it more concise and engaging. Additionally, modify the card component's text size for better visual hierarchy.
Add functionality to expand and collapse the concept list in the PrimerConceptGroupSection component. Introduce tracked properties and actions to manage the expanded state, enhancing user interaction. Update the template to reflect the new behavior with appropriate buttons for collapsing and showing all concepts.
Comment out the unused CTA block in the header component to preserve the layout for future implementation. This change ensures that the code remains clean while allowing for easy reinstatement once the necessary features for concepts and challenges are available.
Changes the comment syntax in the header template from a block comment to an inline comment for better readability. This improves code maintainability and prepares for future updates regarding concepts and challenges.
Skip tests related to the start track button for both anonymous and logged-in users in the track page acceptance tests. This change is made to temporarily disable tests that cannot be executed until we can account for both concepts and challenges. Additionally, update the role attribute for accessibility in the track page component.
Removes commented-out code related to the "Start Track" button and adjusts the layout for better readability. This change simplifies the template and prepares for future enhancements without cluttering the codebase.
Adds a new acceptance test to verify that navigating from the catalog page to the track page does not display a loading indicator. This ensures a smoother user experience by confirming that the track page loads efficiently. Additionally, updates the catalog route to fetch required concept groups and concepts, enhancing data availability for the track page.
This commit removes the unused import of ConceptGroupModel from the track controller. This change helps to clean up the code and improve readability by eliminating unnecessary dependencies.
Update request names in acceptance tests to reflect the new catalog structure. Replace references to "courses listing page" with "catalog" for clarity and consistency across tests. Add missing requests for concepts and concept groups to ensure comprehensive coverage of the catalog functionality. refactor: update language handling in track and catalog routes Refactor the Track and Catalog routes to improve language management. Change the way primer concept groups are accessed by using the language model directly. Remove unnecessary promises for concept groups and concepts, and streamline the fetching of language data. This enhances code clarity and optimizes data retrieval for better performance.
rohitpaulk
force-pushed
the
add-track-primer-concept-group
branch
from
4b45116 to
a666e75
Compare
rohitpaulk
force-pushed
the
add-track-primer-concept-group
branch
from
a666e75 to
58a5138
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/acceptance/track-page/view-track-test.js (1)
159-165: Extract duplicated concept group creation code into a helper function.The concept group creation code is duplicated from the previous test. Consider extracting it into a helper function to improve maintainability.
Create a new helper function:
function createRustPrimerConceptGroup(server) { return server.create('concept-group', { author: server.schema.users.first(), description_markdown: 'Dummy description', concept_slugs: ['tcp-overview', 'network-protocols', 'tcp-overview', 'network-protocols', 'tcp-overview', 'network-protocols'], slug: 'rust-primer', title: 'Rust Primer', }); }app/models/language.ts (1)
255-261: Extract string literals and add test coverage for trackPrimerConceptGroupSlug.The
trackPrimerConceptGroupSlugcomputed property contains hardcoded string literals and lacks test coverage.Apply this diff to extract string literals:
+ const RUST_PRIMER_SLUG = 'rust-primer' as const; get trackPrimerConceptGroupSlug() { if (this.isRust) { - return 'rust-primer'; + return RUST_PRIMER_SLUG; } else { return null; } }Add the following test case:
test('trackPrimerConceptGroupSlug returns correct slug for Rust', function(assert) { const store = this.owner.lookup('service:store'); const rustLanguage = store.createRecord('language', { slug: 'rust' }); assert.strictEqual(rustLanguage.trackPrimerConceptGroupSlug, 'rust-primer', 'Should return rust-primer for Rust'); const nonRustLanguage = store.createRecord('language', { slug: 'go' }); assert.strictEqual(nonRustLanguage.trackPrimerConceptGroupSlug, null, 'Should return null for non-Rust language'); });🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 257-257: app/models/language.ts#L257
Added line #L257 was not covered by tests
[warning] 259-259: app/models/language.ts#L259
Added line #L259 was not covered by testsapp/components/track-page/primer-concept-group-section/index.ts (1)
26-31: Consider immediate state update for collapse action.The
handleCollapseButtonClickaction usesnext()to schedule the state update. Unless there's a specific reason for the delayed update, consider updating the state immediately:@action handleCollapseButtonClick() { - next(() => { - this.conceptListIsExpanded = false; - }); + this.conceptListIsExpanded = false; }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 29-30: app/components/track-page/primer-concept-group-section/index.ts#L29-L30
Added lines #L29 - L30 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/assets/images/heroicons/outline/book-open-outline.svgis excluded by!**/*.svg
📒 Files selected for processing (24)
app/components/track-page/card.hbs(2 hunks)app/components/track-page/course-card-list.ts(2 hunks)app/components/track-page/course-card.hbs(2 hunks)app/components/track-page/header/index.hbs(1 hunks)app/components/track-page/primer-concept-group-section/concept-list-item.hbs(1 hunks)app/components/track-page/primer-concept-group-section/concept-list-item.ts(1 hunks)app/components/track-page/primer-concept-group-section/concept-list.hbs(1 hunks)app/components/track-page/primer-concept-group-section/concept-list.ts(1 hunks)app/components/track-page/primer-concept-group-section/index.hbs(1 hunks)app/components/track-page/primer-concept-group-section/index.ts(1 hunks)app/controllers/track.ts(1 hunks)app/models/language.ts(2 hunks)app/routes/catalog.ts(3 hunks)app/routes/track.ts(1 hunks)app/templates/join-track.hbs(1 hunks)app/templates/track.hbs(1 hunks)tests/acceptance/course-page/attempt-course-stage-test.js(1 hunks)tests/acceptance/course-page/resume-course-test.js(1 hunks)tests/acceptance/course-page/start-course-test.js(1 hunks)tests/acceptance/course-page/switch-repository-test.js(1 hunks)tests/acceptance/course-page/try-other-language-test.js(1 hunks)tests/acceptance/track-page/start-track-test.js(2 hunks)tests/acceptance/track-page/view-track-test.js(2 hunks)tests/pages/track-page.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (20)
- tests/pages/track-page.js
- app/components/track-page/primer-concept-group-section/concept-list-item.hbs
- app/components/track-page/card.hbs
- app/templates/join-track.hbs
- app/components/track-page/course-card.hbs
- app/controllers/track.ts
- app/routes/track.ts
- app/components/track-page/primer-concept-group-section/concept-list.hbs
- tests/acceptance/course-page/start-course-test.js
- tests/acceptance/course-page/switch-repository-test.js
- app/components/track-page/primer-concept-group-section/index.hbs
- tests/acceptance/course-page/attempt-course-stage-test.js
- app/components/track-page/header/index.hbs
- app/components/track-page/course-card-list.ts
- tests/acceptance/course-page/resume-course-test.js
- app/routes/catalog.ts
- tests/acceptance/course-page/try-other-language-test.js
- app/components/track-page/primer-concept-group-section/concept-list-item.ts
- tests/acceptance/track-page/start-track-test.js
- app/templates/track.hbs
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/components/track-page/primer-concept-group-section/concept-list.ts
[warning] 20-21: app/components/track-page/primer-concept-group-section/concept-list.ts#L20-L21
Added lines #L20 - L21 were not covered by tests
[warning] 24-24: app/components/track-page/primer-concept-group-section/concept-list.ts#L24
Added line #L24 was not covered by tests
[warning] 27-27: app/components/track-page/primer-concept-group-section/concept-list.ts#L27
Added line #L27 was not covered by tests
app/components/track-page/primer-concept-group-section/index.ts
[warning] 25-25: app/components/track-page/primer-concept-group-section/index.ts#L25
Added line #L25 was not covered by tests
[warning] 29-30: app/components/track-page/primer-concept-group-section/index.ts#L29-L30
Added lines #L29 - L30 were not covered by tests
[warning] 37-37: app/components/track-page/primer-concept-group-section/index.ts#L37
Added line #L37 was not covered by tests
[warning] 41-41: app/components/track-page/primer-concept-group-section/index.ts#L41
Added line #L41 was not covered by tests
[warning] 43-43: app/components/track-page/primer-concept-group-section/index.ts#L43
Added line #L43 was not covered by tests
app/models/language.ts
[warning] 149-149: app/models/language.ts#L149
Added line #L149 was not covered by tests
[warning] 257-257: app/models/language.ts#L257
Added line #L257 was not covered by tests
[warning] 259-259: app/models/language.ts#L259
Added line #L259 was not covered by tests
🔇 Additional comments (5)
app/components/track-page/primer-concept-group-section/concept-list.ts (1)
18-28: Use findAll() instead of peekAll() for reliable data fetching.The current implementation has potential issues:
peekAll()only returns already loaded records, which could miss concepts if they haven't been loaded yet.- Missing error handling for cases where concepts are not found.
Apply this diff to improve reliability:
get sortedConcepts() { - return this.args.conceptGroup.conceptSlugs.reduce((acc, slug) => { - const concept = this.store.peekAll('concept').find((concept) => concept.slug === slug); - - if (concept) { - acc.push(concept); - } - - return acc; - }, [] as ConceptModel[]); + return Promise.all( + this.args.conceptGroup.conceptSlugs.map(async (slug) => { + try { + const concepts = await this.store.findAll('concept'); + return concepts.find((concept) => concept.slug === slug); + } catch (error) { + console.error(`Failed to load concept ${slug}:`, error); + return null; + } + }) + ).then((concepts) => concepts.filter((concept): concept is ConceptModel => concept !== null)); }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 20-21: app/components/track-page/primer-concept-group-section/concept-list.ts#L20-L21
Added lines #L20 - L21 were not covered by tests
[warning] 24-24: app/components/track-page/primer-concept-group-section/concept-list.ts#L24
Added line #L24 was not covered by tests
[warning] 27-27: app/components/track-page/primer-concept-group-section/concept-list.ts#L27
Added line #L27 was not covered by teststests/acceptance/track-page/view-track-test.js (1)
38-41: Add meaningful assertions for the Rust track page test.The current test uses a dummy assertion that doesn't validate any actual behavior.
Apply this diff to add meaningful assertions:
await visit('/tracks/rust'); -assert.strictEqual(1, 1); // dummy assertion +assert.ok(trackPage.conceptGroup.isVisible, 'Concept group should be visible'); +assert.strictEqual(trackPage.conceptGroup.title, 'Rust Primer', 'Should show correct concept group title'); +assert.strictEqual(trackPage.conceptGroup.concepts.length, 2, 'Should show correct number of concepts');app/models/language.ts (1)
148-150: Add test coverage for isRust computed property.The
isRustcomputed property is not covered by tests.Add the following test case:
test('isRust returns true for Rust language', function(assert) { const store = this.owner.lookup('service:store'); const language = store.createRecord('language', { slug: 'rust' }); assert.true(language.isRust, 'isRust should be true for Rust language'); const nonRustLanguage = store.createRecord('language', { slug: 'go' }); assert.false(nonRustLanguage.isRust, 'isRust should be false for non-Rust language'); });🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 149-149: app/models/language.ts#L149
Added line #L149 was not covered by testsapp/components/track-page/primer-concept-group-section/index.ts (2)
1-18: LGTM! Well-structured imports and type definitions.The imports are comprehensive and the Signature interface is well-defined with proper TypeScript types.
47-51: LGTM! Proper Glint registry declaration.The component is correctly registered in the Glint environment, enabling proper type checking in templates.
| export default class TrackPagePrimerConceptGroupSectionComponent extends Component<Signature> { | ||
| @service declare authenticator: AuthenticatorService; | ||
| @service declare router: RouterService; | ||
|
|
||
| @tracked conceptListIsExpanded = false; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Add tests for component state management.
The conceptListIsExpanded tracked property lacks test coverage. Consider adding tests for:
- Initial state verification
- State changes through actions
- Component rendering in both expanded and collapsed states
🏁 Script executed:
#!/bin/bash
# Check if test file exists
fd -t f "primer-concept-group-section-test.ts" app/components/track-page/Length of output: 73
Add tests for component state management in TrackPagePrimerConceptGroupSectionComponent.
It appears that the test file (app/components/track-page/primer-concept-group-section-test.ts) is missing—meaning the conceptListIsExpanded tracked property currently lacks proper test coverage. Please add tests to verify that:
- The property initially evaluates to
false. - Actions correctly toggle the state.
- The component renders properly in both the expanded and collapsed states.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 25-25: app/components/track-page/primer-concept-group-section/index.ts#L25
Added line #L25 was not covered by tests
| @action | ||
| handleSectionClick() { | ||
| if (this.conceptListIsExpanded) { | ||
| return; | ||
| } | ||
|
|
||
| if (this.authenticator.isAuthenticated) { | ||
| this.conceptListIsExpanded = true; | ||
| } else { | ||
| this.router.transitionTo('concept', this.args.conceptGroup.conceptSlugs[0]!); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add comprehensive tests for section click handler.
The handleSectionClick action contains important business logic for handling authenticated and unauthenticated states. Add tests to cover:
- Early return when already expanded
- Behavior for authenticated users
- Navigation for unauthenticated users
- Validation of concept slug access
🏁 Script executed:
#!/bin/bash
# Description: Check test coverage for authentication paths
# Look for existing tests covering authentication scenarios
rg -l "authenticator\.isAuthenticated.*conceptListIsExpanded" app/components/track-page/
rg -l "router\.transitionTo.*concept" app/components/track-page/Length of output: 221
Enhance Test Coverage for handleSectionClick Action
The handleSectionClick action in app/components/track-page/primer-concept-group-section/index.ts encapsulates critical business logic. Please add unit tests to comprehensively cover the following scenarios:
- Early Return: Verify that if
conceptListIsExpandedis already true, the method returns immediately without further processing. - Authenticated Flow: Ensure that when
this.authenticator.isAuthenticatedis true, the handler correctly setsthis.conceptListIsExpandedto true. - Unauthenticated Flow: Confirm that if the user is not authenticated, the router navigates to the
'concept'route using the first slug fromthis.args.conceptGroup.conceptSlugs. - Concept Slug Validation: Validate that the correct concept slug is accessed and passed during the navigation process.
These tests will safeguard the action's functionality and ensure robust handling of different authentication states.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 37-37: app/components/track-page/primer-concept-group-section/index.ts#L37
Added line #L37 was not covered by tests
[warning] 41-41: app/components/track-page/primer-concept-group-section/index.ts#L41
Added line #L41 was not covered by tests
[warning] 43-43: app/components/track-page/primer-concept-group-section/index.ts#L43
Added line #L43 was not covered by tests
Adds support for the Rust language by introducing the
isRustgetter in the
LanguageModel. Implements a new computed propertytrackPrimerConceptGroupSlugto return the appropriate slug forRust. Updates the
TrackControllerto include a method forretrieving the track primer concept group based on the language.
Enhances the
TrackRouteto query for the concept group whenavailable, ensuring proper data loading for Rust tracks.
Summary by CodeRabbit
New Features
Bug Fixes
Style