Wave 4: angular-eslint migration, remove protractor e2e, headless unit tests by tobydrinkall · Pull Request #418 · COG-GTM/angular2-hn

tobydrinkall · 2026-07-01T23:24:59Z

Summary

Wave 4 of the Angular 9→20 upgrade (test + lint tooling only), based on devin/1782947720-angular20-core-upgrade (#417).

tslint → angular-eslint: added flat eslint.config.js via ng add @angular-eslint/schematics, deleted tslint.json, replaced the lint architect target with @angular-eslint/builder:lint over src/**/*.{ts,html}. Rules requiring codebase-wide migrations are disabled rather than mass-editing code (prefer-standalone, prefer-inject, template/prefer-control-flow, a11y click/focus rules, prefer-const, no-empty-function, etc.). Two small code touches: SettingsService implements OnDestroy and an inline disable for the legacy item selector.
Protractor removal: deleted e2e/, the e2e architect target, and the e2e npm script (protractor is EOL).
Unit tests: new app.component.spec.ts (creation + settings wiring, using provideRouter([]) and CUSTOM_ELEMENTS_SCHEMA) and comment.pipe.spec.ts (discuss / singular / plural). karma.conf.js unchanged; run headlessly with npm test -- --watch=false --browsers=ChromeHeadless.

Verified locally (Node 22): npm run lint, npm run build, and npm test -- --watch=false --browsers=ChromeHeadless (6/6 SUCCESS) all pass. No rxjs or component template/style changes.

Link to Devin session: https://app.devin.ai/sessions/781947dcf306489e92d4ada10c3601aa
Requested by: @tobydrinkall

Devin Review

Status	Commit
⚪ Not started	—

Run Devin Review

…s unit tests Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>

devin-ai-integration · 2026-07-01T23:25:02Z

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

Disable automatic comment, CI, and merge conflict monitoring

devin-ai-integration

Devin Review found 5 potential issues.

devin-ai-integration · 2026-07-01T23:28:08Z

🚩 Production build has sourceMap and namedChunks enabled

The production configuration in angular.json:46-47 has "sourceMap": true and "namedChunks": true. These are typically disabled in production builds for performance and bundle size reasons. This is pre-existing (not changed by this PR), but worth noting as it increases the production bundle size and exposes source code.

(Refers to lines 46-47)

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-07-01T23:28:11Z

+      "@angular-eslint/no-empty-lifecycle-method": "off",
+      "@typescript-eslint/no-empty-function": "off",
+      "@typescript-eslint/no-unused-vars": "off",
+      "@typescript-eslint/no-explicit-any": "off",
+      "@typescript-eslint/no-unsafe-function-type": "off",
+      "prefer-const": "off",


📝 Info: ESLint config disables several useful lint rules

The new ESLint config disables a broad set of rules: @typescript-eslint/no-unused-vars, @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-function-type, @typescript-eslint/no-empty-function, and prefer-const (eslint.config.js:37-42). While these are likely disabled to avoid a flood of errors on the existing codebase, they reduce the value of the linter going forward. Consider setting these to "warn" instead of "off" so new violations are surfaced without blocking the build.

Was this helpful? React with 👍 or 👎 to provide feedback.

Acknowledged — these are set to off deliberately to keep this wave scoped to tooling migration without mass code edits. Tightening them to warn/error is a good follow-up once the standalone/inject migrations land.

devin-ai-integration · 2026-07-01T23:28:14Z

  providedIn: 'root'
 })
-export class SettingsService {
+export class SettingsService implements OnDestroy {


📝 Info: ngOnDestroy on a root-scoped service has minimal practical effect

The PR adds implements OnDestroy to SettingsService (src/app/shared/services/settings.service.ts:8), which is providedIn: 'root'. For root-scoped services, ngOnDestroy is only called when the application's root injector is destroyed (i.e., app shutdown). In a typical single-page app, this means the cleanup code effectively never runs during the app's useful lifetime. The addition is not harmful, but it may create a false sense that the media query listener is being properly managed during the app's lifecycle.

Was this helpful? React with 👍 or 👎 to provide feedback.

Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>

devin-ai-integration

Devin Review found 1 new potential issue.

devin-ai-integration · 2026-07-01T23:35:34Z

    this.setTheme(theme);
  }

+  private boundColorSchemeHandler = this.handleSystemPreferredColorSchemeChange.bind(this);


📝 Info: Field initialization order verified safe under useDefineForClassFields: false

The boundColorSchemeHandler field (src/app/shared/services/settings.service.ts:38) is declared textually after the constructor, which raises a question about whether it's initialized before the constructor body calls subscribeToSystemPreferredColorScheme() at line 20. Because tsconfig.json:13 sets useDefineForClassFields: false, TypeScript hoists ALL field initializers to the top of the constructor (before the explicit body). Verified by compiling a test case with the project's own tsc: the emitted JS places this.boundColorSchemeHandler = ... before the constructor body. This would also be safe under useDefineForClassFields: true (TC39 semantics), where all fields are initialized before the constructor body runs. However, the code's readability would benefit from moving the field declaration above the constructor, since this ordering dependency is non-obvious.

Was this helpful? React with 👍 or 👎 to provide feedback.

Moved the field declaration above the constructor in bdbb176.

Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>

devin-ai-integration

Devin Review found 1 new potential issue.

devin-ai-integration · 2026-07-01T23:39:13Z

📝 Info: Protractor still appears in package-lock.json after e2e removal

The e2e directory, protractor config, and the e2e npm script were all removed, but protractor still appears as a transitive dependency in package-lock.json. This is likely pulled in by @angular-devkit/build-angular which historically depended on it. It's harmless dead weight but could be cleaned up by running npm prune or regenerating the lockfile after confirming no remaining references.

Was this helpful? React with 👍 or 👎 to provide feedback.

Checked: the only protractor references in package-lock.json are inside @angular-devkit/build-angular's optional peerDependencies declaration — protractor is not actually installed as a package (no node_modules/protractor entry). Nothing to prune.

Wave 4: migrate to angular-eslint, remove protractor e2e, add headles…

ac63d6f

…s unit tests Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>

tobydrinkall self-assigned this Jul 1, 2026

devin-ai-integration Bot reviewed Jul 1, 2026

View reviewed changes

Use stable bound handler so media-query listener can be removed

7cebca6

Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>

devin-ai-integration Bot reviewed Jul 1, 2026

View reviewed changes

Move bound handler above constructor; stub ga in AppComponent spec

bdbb176

Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>

devin-ai-integration Bot reviewed Jul 1, 2026

View reviewed changes

devin-ai-integration Bot mentioned this pull request Jul 1, 2026

Core upgrade: Angular 9 → Angular 20 (framework + build config) #417

Open

Uh oh!

Conversation

tobydrinkall commented Jul 1, 2026 • edited by devin-ai-integration Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Devin Review

Uh oh!

devin-ai-integration Bot commented Jul 1, 2026

🤖 Devin AI Engineer

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devin-ai-integration Bot Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devin-ai-integration Bot Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

tobydrinkall commented Jul 1, 2026 •

edited by devin-ai-integration Bot

Loading

devin-ai-integration Bot Jul 1, 2026 •

edited

Loading