Piratefinn/fe 01 foundation and cleanup

[piratefinn]

Summary of Objectives

For some context, recently a branch was created by km where he went ham with Claude Code to deal with a lot of major things. However, the workload is a monster and we wanted human review and touch on it all. Therefore, the work is being split into 6 parts:

Title	Scope
Foundation & Cleanup	RxJS 7 Migration, Code Cleanup, Service Worker Gating, Tooling Updates.
Core Architecture	Backend Abstraction Layer (Interfaces & Implementation).
UI Modernization	Virtual Scroll Table (Replace Canvas), Theme System (Service & Vars), Mobile Responsiveness.
Email Hosting Feature	New Email Hosting Module.
Angular 17 & MDC Upgrade	Angular 17 Core, Material 17 Upgrade, MDC Migration (Heavy CSS).
Angular 19 Finalization	Angular 18 & 19 Upgrade, provideHttpClient, Build System Update, standalone: false.

This is an incredibly terse summary of the plan and the scope of each but splitting it into six steps should make it tolerable to review. By taking the work from km's branch (after I made my own fixes so it passes tests etc.), I am systematically going through each aspect and documenting the changes so we can see what is occurring.

This is the first part of it and the changes made have been described below for a summary, including justifications. No tests additionally break compared to master (I tried fixing them more up but failed and left the effort for later, seeing it as wasted effort after hours of failing). The software itself compiles totally fine and the console is not screaming any additional errors in the browser console while looking at the end rendered product.

Detailed Modifications and Justifications

1. RxJS 7 Migration

• Changes:
  • Replaced 105 instances of the deprecated .toPromise() method with firstValueFrom and lastValueFrom across 27 files.
  • Removed the rxjs-compat package from package.json.
• Justification:
  • firstValueFrom and lastValueFrom explicitly convey intent and offer improved error handling.
  • Ensures future compatibility with RxJS 8 and beyond.
  • Eliminates an unnecessary compatibility dependency.

2. Tooling Updates

• Changes:
  • Added @typescript-eslint/semi (error level) and @typescript-eslint/no-unused-vars (warn level, ignoring underscore-prefixed identifiers) rules to .eslintrc.json.
  • Disabled @typescript-eslint/member-ordering.
  • Applied automatic lint fixes (npm run lint -- --fix) to 31 files, addressing:
    • Missing semicolons.
    • Trailing whitespace in license headers.
    • Addition of Angular lifecycle interfaces (OnInit, OnChanges, DoBootstrap) to component classes and their respective imports.
    • SCSS formatting for readability.
• Justification:
  • Enforces consistent code style and improves code quality by identifying unused variables.
  • Aligns with Angular best practices for lifecycle management.
  • Enhances code maintainability through automated formatting and type-aware linting.

3. Service Worker Gating

• Changes:
  • Modified the Service Worker registration strategy in src/app/app.module.ts from registerWithDelay:5000 to registerWhenStable:30000.
• Justification:
  • Improves application stability by delaying Service Worker registration until Angular's change detection has settled, preventing race conditions and ensuring a fully initialized application for the user.
  • Reduces potential errors from premature Service Worker activation.

4. Code Cleanup

• Changes:
  • src/app/folder/folderlist.component.ts:
    • Implemented OnDestroy lifecycle hook with private destroy$ = new Subject<void>(); and takeUntil(this.destroy$) for proper subscription cleanup (expansionModel.changed and folders subscriptions).
    • Added subscription cleanup in ngOnChanges() to prevent stacking.
    • Introduced null safety to DOM removal (document.getElementById('thedragcanvas')?.remove();).
    • Explicitly marked component with standalone: false.
    • Reverted MatLegacyDialog to MatDialog.
    • Used string literal for 'Inbox' instead of a constant import.
    • Added missing semicolons and proper RxJS imports.
  • src/app/http/progress.service.ts:
    • Added a httpRequestInProgress$: Observable<boolean> = this.httpRequestInProgress.pipe(delay(0)); to prevent ExpressionChangedAfterItHasBeenCheckedError in templates.
• Justification:
  • Prevents ExpressionChangedAfterItHasBeenCheckedError by ensuring proper change detection timing and subscription management.
  • Addresses potential memory leaks through explicit OnDestroy implementation and takeUntil.
  • Improves code maintainability, clarity, and runtime safety with null checks and consistent styling.

5. Styling Cleanup

• Changes:
  • Removed @font-face declarations for "Avenir Next Pro Regular" and "Avenir Next Pro Medium" in src/styles.scss.
  • Reformatted complex SCSS selectors in src/app/changelog/changelog.component.scss for readability.
• Justification:
  • Reduces dependencies on local font files, simplifying deployment.
  • Improves SCSS maintainability and readability by simplifying selector formatting.
  • Confirms consistent spacing values across the application.

Testing Information

• TypeScript compilation: All changes compile successfully without errors.
• Angular compatibility: All implementations are compatible with Angular 16.
• Linting: Automatic lint fixes applied; further manual verification of new rules applied to new code.
• Production readiness: Service Worker remains disabled in development environments.

[antoniskalou]

Good catch on the registerWhenStable vs registerWithDelay, but what's the reasoning behind the change from 5000 to 30000?

[piratefinn]

registerWithDelay:5000 (Hard Delay): The app always waits 5 seconds. Even if the app loads instantly, the Service Worker sits idle for 5s.

registerWhenStable:30000 (Timeout / Safety Net):
Standard Behavior: Registers immediately as soon as the app is stable (e.g., at 2s).
Fallback Behavior: If the app never stabilizes (e.g., due to polling), it forces registration at 30s.

It made sense with difference in strategy with instead of hard polling, though if you think a 5 second timeout safety net would make sense, feel free to suggest.

[antoniskalou]

No, this seems reasonable to me, I was just wondering as to the logic of the time increase. Carry on :)

[castaway]

The reason it was WithDelay not WhenStable was (according to my notes): isStable does not / ever happen, in an app that starts scheduled tasks on startup (which the index worker does) - https://v17.angular.io/api/core/ApplicationRef#is-stable-examples .. I recall that when I tried WhenStable stuff did not work, thus using the WithDelay option

[antoniskalou]

Approved, a very minor comment on one file, otherwise looks good to me 👍

[castaway]

The build system adds+commits this file, PRs should not contain it (unless I am missing something.. ?)

[castaway]

This lost a toPromise which did not get replaced by a firstValueFrom or a lastValueFrom (but there is still an "await" at the beginning).. what should it be?

[juanpeha]

FWIW "monster branch" has

        await lastValueFrom(this.postMessagesToXapianWorker(messagesMissingBodyText.map(searchIndexEntry => {

https://github.com/runbox/runbox7/blob/kirill/claudex-fixes/src/app/xapian/index.worker.ts#L696

[castaway]

The reason it was WithDelay not WhenStable was (according to my notes): isStable does not / ever happen, in an app that starts scheduled tasks on startup (which the index worker does) - https://v17.angular.io/api/core/ApplicationRef#is-stable-examples .. I recall that when I tried WhenStable stuff did not work, thus using the WithDelay option

[castaway]

possibly daft Q: what changes (or does not change?) if we just drop these fonts?

[castaway]

Is this change from an existing PR? feels like I've seen/got this one somewhere..