Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

intro cleanup proto #2472

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions app/components/course-page/header/main-section.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,7 @@
</div>
</div>

{{! For complete steps, we have the "You've completed this step" banner" }}
{{! For in-progress steps, users shouldn't be focused on this area anyway }}
{{#if (not (or (eq @currentStep.status "complete") (eq @currentStep.status "in_progress")))}}
{{#if this.shouldShowProgressIndicator}}
<CoursePage::StepProgressIndicator @step={{@currentStep}} class="mt-2 ml-0.5" />
{{/if}}
</div>
Expand Down
19 changes: 19 additions & 0 deletions app/components/course-page/header/main-section.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,25 @@ export default class MainSectionComponent extends Component<Signature> {
get currentStepAsCourseStageStep(): CourseStageStep {
return this.args.currentStep as CourseStageStep;
}

get shouldShowProgressIndicator(): boolean {
// For complete steps, we have the "You've completed this step" banner
if (this.args.currentStep.status === 'complete') {
return false;
}

// For in-progress steps, users shouldn't be focused on this area anyway
if (this.args.currentStep.status === 'in_progress') {
return false;
}

// On the introduction step, let's not distract the user and instead focus on content
if (this.args.currentStep.type === 'IntroductionStep') {
return false;
}

return true;
}
}

declare module '@glint/environment-ember-loose/registry' {
Expand Down
18 changes: 2 additions & 16 deletions app/components/course-page/header/tab-list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,13 @@ export default class TabListComponent extends Component<Signature> {

get allTabs(): Tab[] {
return {
SetupStep: this.setupTabs,
SetupStep: this.introductionTabs, // Can be the same as IntroductionStep for now
CourseCompletedStep: this.courseCompletedTabs,
CourseStageStep: this.stageTabs,
IntroductionStep: this.introductionTabs,
BaseStagesCompletedStep: this.courseCompletedTabs, // Can be the same as CourseCompletedStep for now
ExtensionCompletedStep: this.courseCompletedTabs, // Can be the same as CourseCompletedStep for now
PreChallengeAssessmentStep: this.introductionTabs, // Can be the same as IntroductionStep for now
}[this.args.currentStep.type];
}

Expand Down Expand Up @@ -74,21 +75,6 @@ export default class TabListComponent extends Component<Signature> {
];
}

get setupTabs(): Tab[] {
return [
{
icon: 'document-text',
name: 'Instructions',
slug: 'instructions',
internalLink: {
route: this.args.currentStep.routeParams.route,
models: this.args.currentStep.routeParams.models,
},
isActive: this.router.currentRouteName === this.args.currentStep.routeParams.route,
},
];
}

get stageTabs(): Tab[] {
const tabs: Tab[] = [];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,8 @@
</:header>

<:expandedSection>
{{#if (eq this.expandedSection.type "SelectLanguageSection")}}
<CoursePage::IntroductionStep::CreateRepositoryCard::SelectLanguageSection
@preferredLanguageSlug={{@preferredLanguageSlug}}
@errorMessage={{this.repositoryCreationErrorMessage}}
@repository={{@repository}}
@onLanguageSelection={{this.handleLanguageSelection}}
/>
{{else if (eq this.expandedSection.type "SelectLanguageProficiencyLevelSection")}}
<CoursePage::IntroductionStep::CreateRepositoryCard::SelectLanguageProficiencyLevelSection
{{#if (eq this.expandedSection.type "SelectLanguageProficiencyLevelSection")}}
<CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::SelectLanguageProficiencyLevelSection
@isDisabled={{bool this.expandedSection.isDisabled}}
@repository={{@repository}}
/>
Expand All @@ -29,13 +22,13 @@
<PrimaryButton class="mt-6" {{on "click" this.expandNextSection}} data-test-next-question-button>↓ Next question</PrimaryButton>
{{/if}}
{{else if (eq this.expandedSection.type "SelectExpectedActivityFrequencySection")}}
<CoursePage::IntroductionStep::CreateRepositoryCard::SelectExpectedActivityFrequencySection
<CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::SelectExpectedActivityFrequencySection
@isDisabled={{bool this.expandedSection.isDisabled}}
@repository={{@repository}}
@onSelect={{this.expandNextSection}}
/>
{{else if (eq this.expandedSection.type "SelectRemindersPreferenceSection")}}
<CoursePage::IntroductionStep::CreateRepositoryCard::SelectRemindersPreferenceSection
<CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::SelectRemindersPreferenceSection
@repository={{@repository}}
@isDisabled={{bool this.expandedSection.isDisabled}}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,6 @@ export default class CreateRepositoryCardComponent extends Component<Signature>

declare module '@glint/environment-ember-loose/registry' {
export default interface Registry {
'CoursePage::IntroductionStep::CreateRepositoryCard': typeof CreateRepositoryCardComponent;
'CoursePage::IntroductionStep::Legacy::CreateRepositoryCard': typeof CreateRepositoryCardComponent;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,6 @@ export default class LanguageButtonComponent extends Component<Signature> {}

declare module '@glint/environment-ember-loose/registry' {
export default interface Registry {
'CoursePage::IntroductionStep::CreateRepositoryCard::LanguageButton': typeof LanguageButtonComponent;
'CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::LanguageButton': typeof LanguageButtonComponent;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,6 @@
</TertiaryButton>
</dd.Trigger>
<dd.Content>
<CoursePage::IntroductionStep::CreateRepositoryCard::RequestLanguageDropdown @course={{@course}} @user={{@user}} @onClose={{dd.actions.close}} />
<CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::RequestLanguageDropdown @course={{@course}} @user={{@user}} @onClose={{dd.actions.close}} />
</dd.Content>
</BasicDropdown>
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,6 @@ export default class RequestLanguageButtonComponent extends Component<Signature>

declare module '@glint/environment-ember-loose/registry' {
export default interface Registry {
'CoursePage::IntroductionStep::CreateRepositoryCard::RequestLanguageButton': typeof RequestLanguageButtonComponent;
'CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::RequestLanguageButton': typeof RequestLanguageButtonComponent;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,6 @@ export default class RequestLanguageDropdownComponent extends Component<Signatur

declare module '@glint/environment-ember-loose/registry' {
export default interface Registry {
'CoursePage::IntroductionStep::CreateRepositoryCard::RequestLanguageDropdown': typeof RequestLanguageDropdownComponent;
'CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::RequestLanguageDropdown': typeof RequestLanguageDropdownComponent;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,6 @@ export default class RequestedLanguagesPromptComponent extends Component<Signatu

declare module '@glint/environment-ember-loose/registry' {
export default interface Registry {
'CoursePage::IntroductionStep::CreateRepositoryCard::RequestedLanguagesPrompt': typeof RequestedLanguagesPromptComponent;
'CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::RequestedLanguagesPrompt': typeof RequestedLanguagesPromptComponent;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,6 @@ export default class SelectExpectedActivityFrequencySectionComponent extends Com

declare module '@glint/environment-ember-loose/registry' {
export default interface Registry {
'CoursePage::IntroductionStep::CreateRepositoryCard::SelectExpectedActivityFrequencySection': typeof SelectExpectedActivityFrequencySectionComponent;
'CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::SelectExpectedActivityFrequencySection': typeof SelectExpectedActivityFrequencySectionComponent;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,6 @@ export default class SelectLanguageProficiencyLevelSectionComponent extends Comp

declare module '@glint/environment-ember-loose/registry' {
export default interface Registry {
'CoursePage::IntroductionStep::CreateRepositoryCard::SelectLanguageProficiencyLevelSection': typeof SelectLanguageProficiencyLevelSectionComponent;
'CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::SelectLanguageProficiencyLevelSection': typeof SelectLanguageProficiencyLevelSectionComponent;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
<span class="ml-3 text-gray-700 dark:text-gray-300">Creating repository...</span>
</div>
{{else}}
<CoursePage::IntroductionStep::CreateRepositoryCard::LanguageButton
<CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::LanguageButton
@language={{@repository.language}}
@releaseStatusIsAlpha={{false}}
@isSelected={{true}}
Expand All @@ -40,33 +40,33 @@
{{else}}
{{#if (and (gt this.preferredLanguages.length 0) (not this.shouldShowNonPreferredLanguages))}}
{{#each this.preferredLanguages as |language|}}
<CoursePage::IntroductionStep::CreateRepositoryCard::LanguageButton
<CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::LanguageButton
@language={{language}}
@releaseStatusIsAlpha={{false}}
@isSelected={{false}}
{{on "click" (fn this.handleLanguageButtonClick language)}}
/>
{{/each}}

<CoursePage::IntroductionStep::CreateRepositoryCard::ShowOtherLanguagesButton {{on "click" this.handleShowOtherLanguagesButtonClick}} />
<CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::ShowOtherLanguagesButton {{on "click" this.handleShowOtherLanguagesButtonClick}} />
{{else}}
{{#each this.orderedLanguageConfigurations as |languageConfiguration|}}
<CoursePage::IntroductionStep::CreateRepositoryCard::LanguageButton
<CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::LanguageButton
@isSelected={{false}}
@language={{languageConfiguration.language}}
@releaseStatusIsAlpha={{languageConfiguration.releaseStatusIsAlpha}}
{{on "click" (fn this.handleLanguageButtonClick languageConfiguration.language)}}
/>
{{/each}}

<CoursePage::IntroductionStep::CreateRepositoryCard::RequestLanguageButton @course={{@repository.course}} @user={{@repository.user}} />
<CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::RequestLanguageButton @course={{@repository.course}} @user={{@repository.user}} />
{{/if}}
{{/if}}
</div>

<AnimatedContainer>
{{#animated-if (gt this.requestedAndUnsupportedLanguages.length 0) use=this.requestedLanguagesPromptTransition}}
<CoursePage::IntroductionStep::CreateRepositoryCard::RequestedLanguagesPrompt
<CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::RequestedLanguagesPrompt
@requestedAndUnsupportedLanguages={{this.requestedAndUnsupportedLanguages}}
/>
{{/animated-if}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,6 @@ export default class SelectLanguageSectionComponent extends Component<Signature>

declare module '@glint/environment-ember-loose/registry' {
export default interface Registry {
'CoursePage::IntroductionStep::CreateRepositoryCard::SelectLanguageSection': typeof SelectLanguageSectionComponent;
'CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::SelectLanguageSection': typeof SelectLanguageSectionComponent;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,6 @@ export default class SelectRemindersPreferenceSectionComponent extends Component

declare module '@glint/environment-ember-loose/registry' {
export default interface Registry {
'CoursePage::IntroductionStep::CreateRepositoryCard::SelectRemindersPreferenceSection': typeof SelectRemindersPreferenceSectionComponent;
'CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::SelectRemindersPreferenceSection': typeof SelectRemindersPreferenceSectionComponent;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@ export default class ShowOtherLanguagesButtonComponent extends Component<Signatu

declare module '@glint/environment-ember-loose/registry' {
export default interface Registry {
'CoursePage::IntroductionStep::CreateRepositoryCard::ShowOtherLanguagesButton': typeof ShowOtherLanguagesButtonComponent;
'CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::ShowOtherLanguagesButton': typeof ShowOtherLanguagesButtonComponent;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,6 @@ export default class WelcomeCardComponent extends Component<Signature> {}

declare module '@glint/environment-ember-loose/registry' {
export default interface Registry {
'CoursePage::IntroductionStep::WelcomeCard': typeof WelcomeCardComponent;
'CoursePage::IntroductionStep::Legacy::WelcomeCard': typeof WelcomeCardComponent;
}
}
18 changes: 18 additions & 0 deletions app/components/course-page/introduction-step/welcome-section.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<div ...attributes>
<div class="prose dark:prose-invert mb-4">
<p>
Welcome to the
{{@repository.course.name}}
challenge!
</p>

{{markdown-to-html @repository.course.descriptionMarkdown}}
</div>

<CoursePage::IntroductionStep::Legacy::CreateRepositoryCard::SelectLanguageSection
@preferredLanguageSlug={{undefined}}
@errorMessage={{undefined}}
@repository={{@repository}}
@onLanguageSelection={{this.handleLanguageSelection}}
/>
</div>
49 changes: 49 additions & 0 deletions app/components/course-page/introduction-step/welcome-section.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import * as Sentry from '@sentry/ember';
import Component from '@glimmer/component';
import LanguageModel from 'codecrafters-frontend/models/language';
import type RepositoryModel from 'codecrafters-frontend/models/repository';
import type RouterService from '@ember/routing/router-service';
import { action } from '@ember/object';
import { service } from '@ember/service';
import { tracked } from '@glimmer/tracking';

export interface Signature {
Element: HTMLDivElement;

Args: {
repository: RepositoryModel;
};
}

export default class WelcomeSectionComponent extends Component<Signature> {
@service declare router: RouterService;

@tracked repositoryCreationErrorMessage?: string;

@action
async handleLanguageSelection(language: LanguageModel) {
this.repositoryCreationErrorMessage = undefined;
this.args.repository.language = language;

try {
await this.args.repository.save(); // TODO: This is kinda slow, investigate ways to make it faster
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Performance concern identified in multiple repository save operations

The same TODO comment about slow repository saves appears in two locations:

  • app/components/course-page/introduction-step/welcome-section.ts
  • app/components/course-page/introduction-step/legacy/create-repository-card.ts

The codebase shows extensive use of repository saves across different components, suggesting this is a systemic performance concern that deserves attention. Since this appears in both current and legacy code paths, it likely affects user experience during course initialization.

🔗 Analysis chain

Address TODO comment about slow repository save

The TODO comment indicates a performance issue that should be investigated.

Would you like me to help create a GitHub issue to track this performance investigation?

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for similar repository save operations and their implementations
ast-grep --pattern 'repository.save()' 

Length of output: 40


Script:

#!/bin/bash
# Let's try a broader search pattern to find repository save operations
ast-grep --pattern 'await $_.save()'

# Also search for any mentions of repository save performance
rg -i "repository.*save.*slow|slow.*repository.*save" -A 2 -B 2

Length of output: 9080

} catch (error) {
this.args.repository.language = undefined;
this.repositoryCreationErrorMessage =
'Failed to create repository, please try again? Contact us at [email protected] if this error persists.';
Sentry.captureException(error);

return;
}
Comment on lines +28 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling and user feedback

The error handling could be improved in several ways:

  1. The error message could be more specific based on the error type
  2. The user should be informed that their language selection was reset
  3. Consider adding a retry mechanism
 try {
   await this.args.repository.save();
 } catch (error) {
   this.args.repository.language = undefined;
+  const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred';
   this.repositoryCreationErrorMessage =
-    'Failed to create repository, please try again? Contact us at [email protected] if this error persists.';
+    `Failed to create repository: ${errorMessage}. Your language selection has been reset. Please try again or contact us at [email protected] if this error persists.`;
   Sentry.captureException(error);
+  
+  // Log additional context for debugging
+  Sentry.setContext("repository", {
+    id: this.args.repository.id,
+    selectedLanguage: language.id
+  });

   return;
 }

Committable suggestion skipped: line range outside the PR's diff.


// this.expandNextSection();

this.router.transitionTo({ queryParams: { repo: this.args.repository.id, track: null } });
}
}

declare module '@glint/environment-ember-loose/registry' {
export default interface Registry {
'CoursePage::IntroductionStep::WelcomeSection': typeof WelcomeSectionComponent;
}
}
2 changes: 1 addition & 1 deletion app/components/course-page/step-content.hbs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<div class="flex-grow flex-shrink min-w-0" ...attributes>
<div class="flex-grow flex-shrink min-w-0 flex" ...attributes>
{{#if (and (not this.shouldHideCompletionNotice) @step.completionNoticeMessage)}}
<CoursePage::CompletedStepNotice
@step={{@step}}
Expand Down
4 changes: 4 additions & 0 deletions app/components/course-page/step-list-item.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@
<div class="rounded-full w-6 h-6 border flex items-center justify-center flex-shrink-0 bg-white dark:bg-gray-850 border-teal-500 shadow-sm">
{{svg-jar "arrow-right" class="w-4 h-4 fill-current text-teal-500"}}
</div>
{{else if (eq @step.type "PreChallengeAssessmentStep")}}
<div class="rounded-full w-6 h-6 border flex items-center justify-center flex-shrink-0 bg-white dark:bg-gray-850 border-teal-500 shadow-sm">
{{svg-jar "dots-horizontal" class="w-4 h-4 fill-current text-teal-500"}}
</div>
{{else if (eq @step.type "CourseStageStep")}}
<div class="rounded-full w-6 h-6 border flex items-center justify-center flex-shrink-0 bg-white dark:bg-gray-850 border-teal-500 shadow-sm">
{{svg-jar "dots-horizontal" class="w-4 h-4 fill-current text-teal-500"}}
Expand Down
17 changes: 17 additions & 0 deletions app/controllers/course/pre-challenge-assessment.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import Controller from '@ember/controller';
import { inject as service } from '@ember/service';
import type AuthenticatorService from 'codecrafters-frontend/services/authenticator';
import type CoursePageStateService from 'codecrafters-frontend/services/course-page-state';
import type { ModelType as CourseRouteModelType } from 'codecrafters-frontend/routes/course';
import type Step from 'codecrafters-frontend/utils/course-page-step-list/step';

export default class PreChallengeAssessmentController extends Controller {
declare model: CourseRouteModelType;

@service declare authenticator: AuthenticatorService;
@service declare coursePageState: CoursePageStateService;

get currentStep(): Step {
return this.coursePageState.currentStep as Step;
}
Comment on lines +14 to +16
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add type safety checks for currentStep getter

The type casting with as Step could lead to runtime errors if coursePageState.currentStep is undefined or of a different type.

Consider adding type guards and error handling:

 get currentStep(): Step {
-  return this.coursePageState.currentStep as Step;
+  const step = this.coursePageState.currentStep;
+  if (!step) {
+    throw new Error('Current step is not defined');
+  }
+  if (!(step instanceof Step)) {
+    throw new Error('Current step is not a valid Step instance');
+  }
+  return step;
 }

Committable suggestion skipped: line range outside the PR's diff.

}
1 change: 1 addition & 0 deletions app/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ Router.map(function () {
// TODO: Add dark mode support
this.route('course', { path: '/courses/:course_slug' }, function () {
this.route('introduction');
this.route('pre-challenge-assessment');
this.route('setup');

// Stage identifier either be '1' (for base stages) or 'ext2:1' (for extension stages)
Expand Down
8 changes: 5 additions & 3 deletions app/templates/course/introduction.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@
/>
{{/if}}

<CoursePage::IntroductionStep::WelcomeCard @repository={{@model.activeRepository}} class="mb-6" />
<CoursePage::IntroductionStep::CreateRepositoryCard
<CoursePage::IntroductionStep::WelcomeSection @repository={{@model.activeRepository}} class="mb-6" />

{{!-- <CoursePage::IntroductionStep::Legacy::WelcomeCard @repository={{@model.activeRepository}} class="mb-6" />
<CoursePage::IntroductionStep::Legacy::CreateRepositoryCard
@repository={{@model.activeRepository}}
@preferredLanguageSlug={{@model.track}}
class="mb-6"
/>
/> --}}
</div>
</CoursePage::CurrentStepCompleteOverlay>
13 changes: 13 additions & 0 deletions app/templates/course/pre-challenge-assessment.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<CoursePage::CurrentStepCompleteOverlay @currentStep={{this.currentStep}}>
<div class="w-full pt-8 pb-32 px-3 md:px-6 lg:px-10">
<div class="prose dark:prose-invert mb-4">
Complete the pre-challenge assessment below to proceed:
</div>

<CoursePage::IntroductionStep::Legacy::CreateRepositoryCard
@repository={{@model.activeRepository}}
@preferredLanguageSlug={{undefined}}
class="mb-6"
/>
</div>
</CoursePage::CurrentStepCompleteOverlay>
Loading
Loading