[Feature Request]: Catch circular dependencies at the PR review

[HardikGoyal2003]

Is your feature request related to a problem? Please describe.

Currently, we lack pre-merge checks for circular dependencies at the PR review level, potentially allowing this undesirable pattern to be merged into the codebase.

Describe the solution (or solutions) you'd like

Incorporate tools like Madge to detect circular dependencies in the TS files.

Describe alternatives you've considered and rejected

No response

Additional context

No response

[yashgoyal0110]

assign please

[HardikGoyal2003]

Hey @yashgoyal0110 Per the guidance at https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia#choosing-a-good-first-issue, please provide an explanation of what your PR will do (with names of files you're changing, what you plan to change in each file, etc.). If it looks good, we can assign you to this issue.

Please also follow the other instructions on that wiki page if you have not yet done so.

Lastly, please note that you shouldn't submit a PR for an issue that is still in "triage needed" status (this includes any issues that you newly file), since such issues might need to be clarified further or rejected. We generally do a triage pass every week. Until this is triaged, we recommend focusing on other issues that have already been triaged. Thanks!

[PradyumnChauhan]

Hey @HardikGoyal2003 , I’d like to work on this feature.

Proposed solution:

• Add Madge as a dev dependency for circular dependency detection in TypeScript files.
• Create a new script (e.g., scripts/run_circular_dependency_checks.py) to wrap Madge and integrate it with Oppia’s linting system.
• Update run_lint_checks.py, js_ts_linter.py, pre-commit and pre-push hooks to include the new check.
• Add a CI job in all_lint_checks.yml to block merges if circular dependencies exist.
• Provide developer guidance and update relevant docs.

Files likely to be changed:

• package.json (add Madge)
• scripts/run_circular_dependency_checks.py
• linters/run_lint_checks.py / linters/js_ts_linter.py
• scripts/pre_commit_hook.py, scripts/pre_push_hook.py
• .github/workflows/all_lint_checks.yml
• Documentation (README / dev guide)

This will ensure early detection and prevent circular dependencies from entering the codebase. Could you please assign this issue to me? I’m ready to implement and test.

[HardikGoyal2003]

@PradyumnChauhan Thanks for showing your interest in this issue. When I filed this issue, I just knew about Madge to find the circular dependencies. Now, I tried to dig a little more into this issue and found a few more things that can also tackle the circular dependencies.

The options are:

• Madge
• Setting showCircularDependencies in angular.json to true for the dev environment
• Using ESLint plugins like import/no-cycle

Now, can you help me understand which one is best to use and why? And try to create a table like the table in the docs, comparing all three options.

https://docs.google.com/document/d/152YdoRSu-kdqG07OOfA7szi-KUDpJegSjYvgb9encdM/edit?tab=t.0

[PradyumnChauhan]

After analyzing the options for circular dependency detection, here’s a quick comparison:

• Madge: Comprehensive detection across all TS/JS files, flexible reporting (JSON/HTML/graphs), CI/CD friendly, integrates well with Oppia’s existing script-based linting.
• Angular showCircularDependencies: Minimal setup, but limited to Angular module dependencies only, no CI reporting, not suitable for full codebase.
• ESLint import/no-cycle: Works for all imports, but adds performance overhead on large codebases and increases complexity of ESLint setup.

✅ Recommendation: Madge – It provides the best balance of coverage, maintainability, and CI/CD integration without breaking existing workflows.

I’d be happy to implement this by adding Madge as a dev dependency, creating a wrapper script for linting, and integrating it into pre-commit hooks + CI. Could you please assign me this issue?

https://docs.google.com/document/d/1FkqbxjJTh-94XyWCsV7AHOHF1AuHGmVSEj7sbNGiAFs/edit?usp=sharing

[HardikGoyal2003]

@PradyumnChauhan Can you please color-code the table so that it becomes a little easier to compare?

Deferring to @jayam04 for further input, as this falls under the Dev Workflow team's scope. PTAL. Thanks!

[PradyumnChauhan]

@HardikGoyal2003 , i have color coded the table ... please give a look @jayam04 . https://docs.google.com/document/d/1FkqbxjJTh-94XyWCsV7AHOHF1AuHGmVSEj7sbNGiAFs/edit?tab=t.0

cc/ @seanlip

[seanlip]

Thanks @PradyumnChauhan.

@HardikGoyal2003 Can you give more clarification on your original issue: in what circumstances did you encounter this? I assume you were trying to merge a PR. Which PR was this? How was the circular dependency eventually detected?

I need to understand the use case better in order to audit the table. Thanks.

[HardikGoyal2003]

Hey @seanlip, when I was researching for my project, I found these:

1) components/forms/custom-forms-directives/image-uploader-modal.component.ts > components/forms/custom-forms-directives/image-uploader.component.ts
2) components/question-directives/question-misconception-editor/question-misconception-editor.component.ts > components/question-directives/question-misconception-editor/tag-misconception-modal-component.ts
3) pages/collection-player-page/collection-player-page.component.ts > pages/collection-player-page/services/collection-player-backend-api.service.ts
4) pages/contributor-dashboard-admin-page/contributor-admin-dashboard-page.component.ts > pages/contributor-dashboard-admin-page/services/contributor-dashboard-admin-stats-backend-api.service.ts
5) pages/contributor-dashboard-admin-page/services/contributor-dashboard-admin-stats-backend-api.service.ts > pages/contributor-dashboard-admin-page/contributor-dashboard-admin-summary.model.ts
6) pages/contributor-dashboard-admin-page/contributor-dashboard-admin-summary.model.ts > pages/contributor-dashboard-admin-page/services/format-contributor-attributes.service.ts
7) pages/contributor-dashboard-page/services/contribution-and-review.service.ts > pages/contributor-dashboard-page/services/contribution-and-review-backend-api.service.ts
8) pages/contributor-dashboard-page/services/contribution-and-review-stats.service.ts > pages/contributor-dashboard-page/services/contribution-and-review-stats-backend-api.service.ts
9) pages/exploration-editor-page/services/exploration-data.service.ts > pages/exploration-editor-page/services/exploration-data-backend-api.service.ts
10) pages/exploration-editor-page/services/user-email-preferences.service.ts > pages/exploration-editor-page/services/user-email-preferences-backend-api.service.ts

Basically, lower-order files are importing the constants from higher-order files, which I think is not a good practice.

EDIT: The Issue was closed by a misclick, I have reopened it.

[jayam04]

@PradyumnChauhan I discussed this with @seanlip, and we agree on Madge. Mainly, because it has wide detection scope, low migration effort, and good performance. Which none other two provide together. So, please go ahead and open a PR.

If you have any other thoughts, feel free to add them.

Thanks!