feat(autofix-result): refactor logstream handling with modifier #3188
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Bundle ReportChanges will decrease total bundle size by 2.41kB (-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
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
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.
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.
Remove the unused `action` import from the logstream-did-update modifier to clean up the code and avoid unnecessary dependencies.
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.
rohitpaulk force-pushed the refactor-autofixresult-logstream-handling branch from 68646db to 06a96d1 Compare Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Replace manual logstream management in AutofixResult component with a
new
logstream-did-updatemodifier that handles subscription lifecycleautomatically. 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-updatemodifier that encapsulates subscribing andunsubscribing to logstreams and invokes a callback on updates.
Overall, these changes improve code clarity, correctness, and maintainability
around logstream update handling.
Note
Extract a
logstream-did-updatemodifier to manage logstream subscriptions and simplifyAutofixResultto consume tracked content.autofix-result.hbs): Replace manual lifecycle modifiers with{{logstream-did-update this.handleLogstreamDidUpdate @autofixRequest.logstreamId}}; renderAnsiStreamfrom trackedthis.logstreamContent.autofix-result.ts): Remove manualLogstreammanagement (subscribe/unsubscribe and element hooks); addhandleLogstreamDidUpdateto setlogstreamContentand reload theautofixRequest; drop injectedstore/actionCableConsumerusage.app/modifiers/logstream-did-update.ts: encapsulatesLogstreamsubscription lifecycle, cleans up on re-run/destroy, and invokes a callback on updates; registers in Glint registry.app/utils/logstream.ts: remove earlyisSubscribedguard insubscribeTaskto allow re-subscription after cleanup.Written by Cursor Bugbot for commit 06a96d1. This will update automatically on new commits. Configure here.