From 25f40112b65c57548328286be87e65fc8410bd9a Mon Sep 17 00:00:00 2001 From: Paul Kuruvilla Date: Thu, 16 Oct 2025 20:29:13 -0700 Subject: [PATCH 1/5] feat(autofix-result): refactor logstream handling with modifier Replace manual logstream management in AutofixResult component with a new `logstream-did-update` modifier that handles subscription lifecycle automatically. This simplifies the component by removing explicit setup, teardown, and state syncing of Logstream instances. Update the template to use the modifier for logstream updates and track logstream content reactively. This ensures better separation of concerns and reduces boilerplate. Add `logstream-did-update` modifier that encapsulates subscribing and unsubscribing to logstreams and invokes a callback on updates. Overall, these changes improve code clarity, correctness, and maintainability around logstream update handling. --- .../autofix-section/autofix-result.hbs | 11 ++--- .../autofix-section/autofix-result.ts | 33 +++---------- app/modifiers/logstream-did-update.ts | 47 +++++++++++++++++++ 3 files changed, 56 insertions(+), 35 deletions(-) create mode 100644 app/modifiers/logstream-did-update.ts diff --git a/app/components/course-page/test-results-bar/autofix-section/autofix-result.hbs b/app/components/course-page/test-results-bar/autofix-section/autofix-result.hbs index 40c68f2218..3b73622685 100644 --- a/app/components/course-page/test-results-bar/autofix-section/autofix-result.hbs +++ b/app/components/course-page/test-results-bar/autofix-section/autofix-result.hbs @@ -35,14 +35,9 @@ {{#if (eq @autofixRequest.status "in_progress")}}
-
- {{#if this.logstream}} - +
+ {{#if this.logstreamContent}} + {{/if}}
diff --git a/app/components/course-page/test-results-bar/autofix-section/autofix-result.ts b/app/components/course-page/test-results-bar/autofix-section/autofix-result.ts index 6d661b27de..8062656ca1 100644 --- a/app/components/course-page/test-results-bar/autofix-section/autofix-result.ts +++ b/app/components/course-page/test-results-bar/autofix-section/autofix-result.ts @@ -1,12 +1,10 @@ import AnalyticsEventTrackerService from 'codecrafters-frontend/services/analytics-event-tracker'; -import type Store from '@ember-data/store'; import { action } from '@ember/object'; import { service } from '@ember/service'; import Component from '@glimmer/component'; import { tracked } from '@glimmer/tracking'; -import Logstream from 'codecrafters-frontend/utils/logstream'; import type AutofixRequestModel from 'codecrafters-frontend/models/autofix-request'; -import type ActionCableConsumerService from 'codecrafters-frontend/services/action-cable-consumer'; +import type Logstream from 'codecrafters-frontend/utils/logstream'; interface Signature { Element: HTMLDivElement; @@ -17,40 +15,21 @@ interface Signature { } export default class AutofixResult extends Component { - @service declare store: Store; - @service declare actionCableConsumer: ActionCableConsumerService; @service declare analyticsEventTracker: AnalyticsEventTrackerService; - @tracked logstream: Logstream | null = null; @tracked shouldShowFullLog = false; + @tracked logstreamContent: string | null = null; @tracked diffIsBlurred = true; @action - handleDidUpdateAutofixRequestLogstreamId() { - if (this.logstream && this.args.autofixRequest.logstreamId !== this.logstream.logstreamId) { - this.logstream.unsubscribe(); - this.handleMarkdownStreamElementInserted(); // create a new logstream - } - } + handleLogstreamDidUpdate(logstream: Logstream): void { + this.logstreamContent = logstream.content; - @action - handleLogstreamDidPoll(): void { + // TODO: See if we're doing this too often? + // Ensure we also reload the autofix request to see whether the status has changed this.args.autofixRequest.reload(); } - @action - handleMarkdownStreamElementInserted() { - this.logstream = new Logstream(this.args.autofixRequest.logstreamId, this.actionCableConsumer, this.store, this.handleLogstreamDidPoll); - this.logstream.subscribe(); - } - - @action - handleWillDestroyMarkdownStreamElement() { - if (this.logstream) { - this.logstream.unsubscribe(); - } - } - @action toggleBlur() { if (this.diffIsBlurred) { diff --git a/app/modifiers/logstream-did-update.ts b/app/modifiers/logstream-did-update.ts new file mode 100644 index 0000000000..a290954f86 --- /dev/null +++ b/app/modifiers/logstream-did-update.ts @@ -0,0 +1,47 @@ +// Invokes a callback when a logstream is updated. +// Usage:
+import type ActionCableConsumerService from 'codecrafters-frontend/services/action-cable-consumer'; +import type Store from '@ember-data/store'; +import Logstream from 'codecrafters-frontend/utils/logstream'; +import Modifier from 'ember-modifier'; +import { inject as service } from '@ember/service'; +import { registerDestructor } from '@ember/destroyable'; +import { action } from '@ember/object'; + +interface Signature { + Args: { + Positional: [(logstream: Logstream) => void, string]; + }; +} + +export default class LogstreamDidUpdateModifier extends Modifier { + @service declare actionCableConsumer: ActionCableConsumerService; + @service declare store: Store; + + callback?: (logstream: Logstream) => void; + logstream?: Logstream; + + @action + handleLogstreamDidPoll(): void { + this.callback!(this.logstream!); + } + + modify(_element: HTMLElement, [callback, logstreamId]: Signature['Args']['Positional']) { + this.logstream = new Logstream(logstreamId, this.actionCableConsumer, this.store, this.handleLogstreamDidPoll); + this.callback = callback; + + console.log(`subscribing to logstream#${logstreamId}`); + this.logstream.subscribe(); + + registerDestructor(this, () => { + console.log(`unsubscribing from logstream#${logstreamId}`); + this.logstream?.unsubscribe(); + }); + } +} + +declare module '@glint/environment-ember-loose/registry' { + export default interface Registry { + 'logstream-did-update': typeof LogstreamDidUpdateModifier; + } +} From ea15d50edb2d616a49891db4d2690f104b149a7d Mon Sep 17 00:00:00 2001 From: Paul Kuruvilla Date: Thu, 16 Oct 2025 20:48:56 -0700 Subject: [PATCH 2/5] fix(logstream): remove redundant subscription check Eliminate early return on isSubscribed in subscribeTask to allow resubscription logic to proceed. This change fixes an issue where the subscription could not be re-established once terminated, improving reliability of the log stream handling. --- app/utils/logstream.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/utils/logstream.ts b/app/utils/logstream.ts index 5da6785e7e..28585163b4 100644 --- a/app/utils/logstream.ts +++ b/app/utils/logstream.ts @@ -35,10 +35,6 @@ export default class Logstream { } subscribeTask = task({ drop: true }, async (): Promise => { - if (this.isSubscribed) { - return; - } - this.isTerminated = false; this.isSubscribed = false; From f37dd4e5459ffc790ba8766ccd4ca2c2125f71ea Mon Sep 17 00:00:00 2001 From: Paul Kuruvilla Date: Thu, 16 Oct 2025 21:19:55 -0700 Subject: [PATCH 3/5] refactor(logstream-did-update): improve resource management Register a destructor for the modifier to ensure the Logstream is properly unsubscribed when the modifier is destroyed. Replace ad-hoc unsubscribe logic in modify() with a dedicated cleanup function. Use a lambda callback to invoke the provided callback with the current logstream, removing unnecessary stored callback references and improving code clarity. --- app/modifiers/logstream-did-update.ts | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/app/modifiers/logstream-did-update.ts b/app/modifiers/logstream-did-update.ts index a290954f86..aa21e48f62 100644 --- a/app/modifiers/logstream-did-update.ts +++ b/app/modifiers/logstream-did-update.ts @@ -3,10 +3,11 @@ import type ActionCableConsumerService from 'codecrafters-frontend/services/action-cable-consumer'; import type Store from '@ember-data/store'; import Logstream from 'codecrafters-frontend/utils/logstream'; -import Modifier from 'ember-modifier'; +import Modifier, { type ArgsFor } from 'ember-modifier'; import { inject as service } from '@ember/service'; import { registerDestructor } from '@ember/destroyable'; import { action } from '@ember/object'; +import type { Owner } from '@ember/test-helpers/build-owner'; interface Signature { Args: { @@ -14,29 +15,28 @@ interface Signature { }; } +function cleanup(instance: LogstreamDidUpdateModifier) { + if (instance.logstream) { + instance.logstream.unsubscribe(); + } +} + export default class LogstreamDidUpdateModifier extends Modifier { @service declare actionCableConsumer: ActionCableConsumerService; @service declare store: Store; - callback?: (logstream: Logstream) => void; logstream?: Logstream; - @action - handleLogstreamDidPoll(): void { - this.callback!(this.logstream!); + constructor(owner: unknown, args: ArgsFor) { + super(owner as Owner, args); + registerDestructor(this, cleanup); } modify(_element: HTMLElement, [callback, logstreamId]: Signature['Args']['Positional']) { - this.logstream = new Logstream(logstreamId, this.actionCableConsumer, this.store, this.handleLogstreamDidPoll); - this.callback = callback; + cleanup(this); - console.log(`subscribing to logstream#${logstreamId}`); + this.logstream = new Logstream(logstreamId, this.actionCableConsumer, this.store, () => callback(this.logstream!)); this.logstream.subscribe(); - - registerDestructor(this, () => { - console.log(`unsubscribing from logstream#${logstreamId}`); - this.logstream?.unsubscribe(); - }); } } From 9ba6ce5e6a6ede480b13771d5d032080f32070ef Mon Sep 17 00:00:00 2001 From: Paul Kuruvilla Date: Thu, 16 Oct 2025 21:23:07 -0700 Subject: [PATCH 4/5] refactor(logstream): remove unused action import Remove the unused `action` import from the logstream-did-update modifier to clean up the code and avoid unnecessary dependencies. --- app/modifiers/logstream-did-update.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/app/modifiers/logstream-did-update.ts b/app/modifiers/logstream-did-update.ts index aa21e48f62..67a13f6316 100644 --- a/app/modifiers/logstream-did-update.ts +++ b/app/modifiers/logstream-did-update.ts @@ -6,7 +6,6 @@ import Logstream from 'codecrafters-frontend/utils/logstream'; import Modifier, { type ArgsFor } from 'ember-modifier'; import { inject as service } from '@ember/service'; import { registerDestructor } from '@ember/destroyable'; -import { action } from '@ember/object'; import type { Owner } from '@ember/test-helpers/build-owner'; interface Signature { From 521128d68e9dd77281525960922b66fdae56ba13 Mon Sep 17 00:00:00 2001 From: Paul Kuruvilla Date: Thu, 16 Oct 2025 21:30:04 -0700 Subject: [PATCH 5/5] fix(logstream): clear logstream reference on cleanup Set the logstream property to undefined during cleanup to prevent potential memory leaks or stale references. Also, adjust the callback invocation in modify for clearer structure while keeping the same behavior. --- app/modifiers/logstream-did-update.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/modifiers/logstream-did-update.ts b/app/modifiers/logstream-did-update.ts index 67a13f6316..66cee61f43 100644 --- a/app/modifiers/logstream-did-update.ts +++ b/app/modifiers/logstream-did-update.ts @@ -17,6 +17,7 @@ interface Signature { function cleanup(instance: LogstreamDidUpdateModifier) { if (instance.logstream) { instance.logstream.unsubscribe(); + instance.logstream = undefined; } } @@ -34,7 +35,10 @@ export default class LogstreamDidUpdateModifier extends Modifier { modify(_element: HTMLElement, [callback, logstreamId]: Signature['Args']['Positional']) { cleanup(this); - this.logstream = new Logstream(logstreamId, this.actionCableConsumer, this.store, () => callback(this.logstream!)); + this.logstream = new Logstream(logstreamId, this.actionCableConsumer, this.store, () => { + callback(this.logstream!); + }); + this.logstream.subscribe(); } }