Skip to content

refactor(frontend): standardize menu.component subscription cleanup#5324

Merged
Yicong-Huang merged 7 commits into
apache:mainfrom
Ma77Ball:fix/menu-subscription-cleanup
Jun 6, 2026
Merged

refactor(frontend): standardize menu.component subscription cleanup#5324
Yicong-Huang merged 7 commits into
apache:mainfrom
Ma77Ball:fix/menu-subscription-cleanup

Conversation

@Ma77Ball

@Ma77Ball Ma77Ball commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

  • Removed the redundant computingUnitStatusSubscription field (and its .add() wrapper and ngOnDestroy unsubscribe) in menu.component.ts: the inner stream is already piped through untilDestroyed(this), so the manual aggregator did nothing.
  • Replaced the manually managed durationUpdateSubscription with a declarative switchMap pipe, so each ExecutionDurationUpdateEvent cancels the previous 1s timer and starts a new one only while running, instead of imperatively unsubscribing and reassigning a field.
  • Dropped the now-unused Subscription import, leaving the component on a single consistent cleanup style (untilDestroyed).

Any related issues, documentation, or discussions?

Closes: #5323

How was this PR tested?

  • Ran ng test --include='src/app/workspace/component/menu/menu.component.spec.ts': all 42 tests pass.
  • Behavior-preserving refactor: same elapsed-timer cadence, same base-duration updates, same teardown on destroy.

Was this PR authored or co-authored using generative AI tooling?

Co-authored with Claude Opus 4.7 in compliance with ASF

@github-actions github-actions Bot added fix frontend Changes related to the frontend GUI labels Jun 3, 2026
@codecov-commenter

codecov-commenter commented Jun 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.31%. Comparing base (8a4cb29) to head (00f7b8a).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5324      +/-   ##
============================================
- Coverage     51.62%   51.31%   -0.31%     
+ Complexity     2420     2385      -35     
============================================
  Files          1056     1053       -3     
  Lines         41046    40515     -531     
  Branches       4407     4317      -90     
============================================
- Hits          21190    20791     -399     
+ Misses        18617    18525      -92     
+ Partials       1239     1199      -40     
Flag Coverage Δ *Carryforward flag
access-control-service 41.89% <ø> (ø) Carriedforward from 34be37d
agent-service 33.76% <ø> (ø) Carriedforward from 34be37d
amber 51.65% <ø> (-0.58%) ⬇️ Carriedforward from 34be37d
computing-unit-managing-service 0.00% <ø> (-1.39%) ⬇️ Carriedforward from 34be37d
config-service 0.00% <ø> (-54.69%) ⬇️ Carriedforward from 34be37d
file-service 38.42% <ø> (ø) Carriedforward from 34be37d
frontend 46.30% <100.00%> (-0.03%) ⬇️
pyamber ?
python 90.84% <ø> (ø) Carriedforward from 34be37d
workflow-compiling-service 57.25% <ø> (-1.15%) ⬇️ Carriedforward from 34be37d

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Yicong-Huang

Copy link
Copy Markdown
Contributor

@Ma77Ball please add tests to verify the described issue does not happen.

@Ma77Ball

Ma77Ball commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

/request-review @Yicong-Huang

@github-actions github-actions Bot requested a review from Yicong-Huang June 3, 2026 20:43
@Yicong-Huang Yicong-Huang added this pull request to the merge queue Jun 6, 2026
Merged via the queue into apache:main with commit 6a9437e Jun 6, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix frontend Changes related to the frontend GUI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

menu.component.ts mixes manual Subscription cleanup with untilDestroyed

3 participants