[OP-20342] Frontend typings: foundations by myabc · Pull Request #25657 · opf/openproject

myabc · 2026-09-28T21:24:50Z

Ticket

https://community.openproject.org/wp/OP-20342

What are you trying to accomplish?

Lays the groundwork for OP-20341 (zero ESLint typing violations in frontend application code) by removing shared sources of any.

Exempts specs and test helpers from no-explicit-any and the no-unsafe-* rules; the typing effort targets application code.
Removes the spec and test-setup disables that the exemption makes redundant.
Removes unused global declarations (api.v3 Result/Collection/Duration, Factory, op.QueryParams) and types the tablesorter jQuery plugin.
Types HookService callbacks by hook name.
Replaces Function types with real signatures.

Application-code typing violations drop from 1,203 to 1,174.

What approach did you choose and why?

HookService gets a HookSignatures map for the ten hooks the core registers and calls. Plugins register and call hooks by arbitrary name, so both methods keep an open fallback overload; the fallback excludes the known names, so a mismatched callback or argument for a known hook fails to compile. The spec pins this down with @ts-expect-error cases. Typing the hooks exposed that the bulk context menu actions were inferred narrower than WorkPackageAction.

The dynamic component output maps use ng-dynamic-component's EventHandler, which is what ndcDynamicOutputs accepts.

Stacked on #25656 because both touch the same declaration files; GitHub retargets this to dev once #25656 merges. The Function.$link global and the dom-plane/onboarding declarations stay for the HAL resources and third-party tickets, which type their consumers.

AI involvement

Directed – I specified the requirements and AI implemented most of it; I validated via testing rather than a full line-by-line review.

Merge checklist

Added/updated tests
Added/updated documentation in Lookbook (patterns, previews, etc)
Tested major browsers (Chrome, Firefox, Edge, ...)

Browser JavaScript diff (1 files, type-only changes omitted)

Each changed browser file (TypeScript and templates, excluding specs, test helpers and declaration files) is transpiled on its own with frontend/tsconfig.json, before and after; this is the emitted JavaScript that differs.

diff --git frontend/src/app/features/plugins/hook-service.js
index d8aee60..45c701e 100644
--- frontend/src/app/features/plugins/hook-service.js
+++ frontend/src/app/features/plugins/hook-service.js
@@ -16,8 +16,8 @@ let HookService = class HookService {
     call(id, ...params) {
         const results = [];
         if (this.hooks[id]) {
-            for (let x = 0; x < this.hooks[id].length; x++) {
-                const result = this.hooks[id][x](...params);
+            for (const hook of this.hooks[id]) {
+                const result = hook(...params);
                 if (result) {
                     results.push(result);
                 }

Copilot

Copilot review overview

🔵 Needs a closer look

The plugin-facing hook bridge does not yet enforce the known hook signatures.

Review effort: Lite
Findings: None

What changed in this PR

This PR strengthens frontend typings and reduces shared any/Function usage as groundwork for eliminating application typing violations.

Changes:

Types tablesorter integrations and dynamic component outputs.
Adds typed HookService signatures and validation tests.
Removes obsolete declarations and exempts test code from selected lint rules.

File	Summary
`frontend/src/typings/shims.d.ts`	Types jQuery tablesorter APIs.
`frontend/src/typings/open-project.typings.d.ts`	Removes unused global declarations.
`frontend/src/stimulus/controllers/dynamic/reporting/page.controller.ts`	Uses updated tablesorter typings.
`frontend/src/app/shared/components/fields/edit/field-types/select-edit-field/select-edit-field.component.ts`	Types dynamic output handlers.
`frontend/src/app/features/work-packages/routing/partitioned-query-space-page/partitioned-query-space-page.component.ts`	Types dynamic component outputs.
`frontend/src/app/features/work-packages/components/wp-table/context-menu-helper/wp-context-menu-helper.service.ts`	Types bulk actions.
`frontend/src/app/features/work-packages/components/wp-list/wp-list-checksum.service.ts`	Replaces a `Function` callback type.
`frontend/src/app/features/plugins/hook-service.ts`	Adds typed hook signatures and custom-hook fallback.
`frontend/src/app/features/plugins/hook-service.spec.ts`	Tests known-hook signature enforcement.
`frontend/eslint.config.mjs`	Exempts tests and helpers from selected unsafe typing rules.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

github-actions · 2026-09-29T10:31:47Z

Warning

Flaky specs

rspec ./spec/features/projects/creation_wizard/project_creation_wizard_spec.rb[1:8:2]
rspec ./spec/features/projects/creation_wizard/project_creation_wizard_spec.rb[1:11:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #25657, linked for reference only):

- `rspec ./spec/features/projects/creation_wizard/project_creation_wizard_spec.rb[1:8:2]`
- `rspec ./spec/features/projects/creation_wizard/project_creation_wizard_spec.rb[1:11:1]`

Treat this as a standalone task, unrelated to PR #25657. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #25657 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

Turns off no-explicit-any and the no-unsafe-* rules for specs and the test helpers that are kept out of the production bundle. Test doubles and expectation helpers routinely work with loosely typed values, and the typing effort under OP-20341 targets application code only. The two no-unsafe exemptions that lived in the vitest spec block move into the new block, which covers the helper files as well. Removes the inline disables in specs and test setup that the exemption makes redundant. https://community.openproject.org/wp/OP-20342

Removes the api.v3 Result, Collection and Duration interfaces, the Factory global and the op.QueryParams namespace. Nothing references them, and most declared their members as any. https://community.openproject.org/wp/OP-20342

Declares the tablesorter call and its language defaults, and the metadata plugin slot the reporting page clears, instead of any. The reporting controller no longer needs its inline disables for them. https://community.openproject.org/wp/OP-20342

Adds a HookSignatures map for the hooks the core registers and calls, so register and call check callback parameters and return typed results instead of any. Arbitrary hook names still fall back to an open signature, because plugins register and call hooks by name. The fallback excludes the known names, so a mismatched callback or argument for a known hook fails to compile; the spec pins that down. The bulk context menu actions are now declared as WorkPackageAction, which the typed hook result exposed as narrower than its use. https://community.openproject.org/wp/OP-20342

Types the dynamic component output maps with ng-dynamic-component's EventHandler, which is what ndcDynamicOutputs accepts, and the checksum callback as a plain thunk. The unconstrained Function type accepted any callable and let calls through unchecked. https://community.openproject.org/wp/OP-20342

github-actions · 2026-09-29T18:56:59Z

Warning

Flaky specs

rspec ./spec/features/notifications/navigation_spec.rb[1:1:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #25657, linked for reference only):

- `rspec ./spec/features/notifications/navigation_spec.rb[1:1:1]`

Treat this as a standalone task, unrelated to PR #25657. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #25657 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

lwassermann

Great initiative 👍 💯

This is a bit sprinkled about, but since the changes are all for static checks, I'm not worried.

lwassermann · 2026-09-30T15:41:30Z

+  workPackageNewInitialization:(change:WorkPackageChangeset) => void;
+  workPackageSingleContextMenu:() => WorkPackageAction;
+  workPackageTableContextMenu:() => WorkPackageAction;
+}


Updating this list would now be up to the registrees of the Hook service, wouldn't it?

In my understanding, this pattern where modules register themselves is an attempt to decouple the specific implementations from being depended on by the central registry. In the past, I had the impression that it hides more than it shows. Since we already have the specific types here, maybe we can also remove the registering and just register them all explicitly when this module loads.

Or, if that is not a feasible refactoring, instead of having all the signatures here, every module that registers itself would also extend the HookSignatures interface? Or does this run into other problems? I've read that the core difference between type A = {} and interface A {} is that the interface is open and can be extended, but I'm not sure I've seen that in action. So this might not be possible 😅.

Updating this list would now be up to the registrees of the Hook service, wouldn't it?

Not quite. The contract lives with the caller.

Adding a new hook.call(...) will mean adding its signature here, Plugins registering callbacks will never touch this list. Unknown hook ids will still fall through to the untyped overload, so nothing breaks.

Declaration merging would work (declare module 'core-app/features/plugins/hook-service' { interface HookSignatures { ... } } next to each call site) and would drop the imports here. But it scatters the hooks back across the codebase, and I think the central list is what makes the indirection visible, which is your concern.

Replacing self-registration with explicit wiring is worth doing, but it's out of scope for a typing PR IMO: two plugins register through pluginContext.hooks (untyped, outside this interface), and four hooks (gridWidgets, workPackageAttachmentUploadComponent, workPackageBulkContextMenu, workPackageNewInitialization) have no in-repo registrant at all. Removing the dead ones would be part of that decision. I'd prefer to tackle it in a follow-up PR, if that works for you?

I've created OP-20381 to follow up on this. Feedback appreciated! 🙏🏻

myabc added this pull request to stack #25658 September 28, 2026 21:25

myabc requested a lite review from Copilot September 28, 2026 21:25

Copilot started reviewing on behalf of myabc September 28, 2026 21:25 View session

myabc marked this pull request as ready for review September 28, 2026 21:26

myabc added the eslint label Sep 28, 2026

github-actions Bot added the ai: Directed 🪄 A human specified the requirements and AI implemented most of it; They validated via testing. label Sep 28, 2026

myabc added javascript Pull requests that update Javascript code needs review labels Sep 28, 2026

Copilot AI reviewed Sep 28, 2026

View reviewed changes

This was referenced Sep 28, 2026

[OP-20344] Frontend typings: HAL resources #25660

Open

[OP-20348] Frontend typings: remaining modules #25666

Open

myabc force-pushed the implementation/op-20342-eslint-typing-fix-foundations branch from b3c3e4d to 61400e7 Compare September 29, 2026 09:47

myabc added this to the 18.0.x milestone Sep 29, 2026

Base automatically changed from housekeeping/eslint-fixes to dev September 29, 2026 18:29

myabc added 5 commits September 29, 2026 19:29

[OP-20342] Remove unused global type declarations

a290bdf

Removes the api.v3 Result, Collection and Duration interfaces, the Factory global and the op.QueryParams namespace. Nothing references them, and most declared their members as any. https://community.openproject.org/wp/OP-20342

myabc force-pushed the implementation/op-20342-eslint-typing-fix-foundations branch from 61400e7 to 661dab8 Compare September 29, 2026 18:29

lwassermann requested changes Sep 30, 2026

View reviewed changes

myabc requested a review from lwassermann September 30, 2026 17:00

Navigation Menu

[OP-20342] Frontend typings: foundations - #25657

[OP-20342] Frontend typings: foundations#25657
myabc wants to merge 5 commits into
devfrom
implementation/op-20342-eslint-typing-fix-foundations

myabc commented Sep 28, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

github-actions Bot commented Sep 29, 2026

Uh oh!

github-actions Bot commented Sep 29, 2026

Uh oh!

lwassermann left a comment

Uh oh!

lwassermann Sep 30, 2026

Uh oh!

myabc Sep 30, 2026

Uh oh!

myabc Sep 30, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Conversation

myabc commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

What are you trying to accomplish?

What approach did you choose and why?

AI involvement

Merge checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

github-actions Bot commented Sep 29, 2026

Uh oh!

github-actions Bot commented Sep 29, 2026

Uh oh!

lwassermann left a comment

Choose a reason for hiding this comment

Uh oh!

lwassermann Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

myabc Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

myabc Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

myabc commented Sep 28, 2026 •

edited

Loading