[RFR] Revert PR 1679

[nachandr]

#1679 was just a WIP PR to see if behavior had changed in MTA 8.0.0.
The explicit wait is still required for automation to pass.

Summary by CodeRabbit

• Tests
  • Improved reliability of the Application Inventory end-to-end flow by introducing a brief wait before validating the header after a forced reload, reducing test flakiness in slower environments.
  • Streamlined the assertion timing to make tests more consistent across runs.
  • Added clarifying comments tied to a known issue to aid future maintenance.
  • No impact on product functionality or performance; changes are limited to test stability.

[coderabbitai]

Walkthrough

In cypress/e2e/models/migration/applicationinventory/application.ts, the Application.open(forceReload) method is modified. When forceReload is true, a 10-second cy.wait(10 * SEC) is added before asserting the header, and the explicit 10-second timeout option on the h1 selector is removed. A comment referencing Bug MTA-3812 precedes the wait. No public or exported signatures are changed.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• Remove explicit wait in Application.open() #1679 — Also changes Application.open(forceReload) but in the opposite manner (removes global wait, adds timeout on h1), indicating a direct conflict.
• [RFR] Add bug marker #1655 — Similar change in the same method, adding a 10s cy.wait and removing the h1 timeout with a related bug comment.

Suggested reviewers

• sshveta

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title "[RFR] Revert PR 1679" is concise and directly communicates the primary action (reverting PR #1679), which aligns with the branch name and PR objective and therefore accurately summarizes the main change for reviewers.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.
Description Check	✅ Passed	The PR description succinctly explains this revert and references the prior WIP PR and the reason that an explicit wait remains required. It includes the repository's default template comments but does not enumerate changed files or assign reviewers as suggested by the template. Given that it conveys the core rationale and testing outcome, the description is mostly complete for review but could be improved with a small amount of additional detail.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

• Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
• Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 90a58cb and 43b3fb5.

📒 Files selected for processing (1)

• cypress/e2e/models/migration/applicationinventory/application.ts (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

cypress/e2e/models/migration/applicationinventory/application.ts (1)

cypress/e2e/types/constants.ts (2)

• SEC (129-129)
• applicationInventory (20-20)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: e2e-main / e2e-ui-integration-tests

[coderabbitai]

⚠️ Potential issue

Avoid fixed waits; keep an explicit timeout on the header to reduce flakiness

A hard 10s wait slows tests and the removal of the explicit timeout on the header weakens resilience. Default Cypress timeout (~4s) may still flake after the 10s wait. Keep the temporary wait if needed for MTA-3812, but reintroduce a longer explicit timeout consistent with the non-reload path.

Apply this diff:

-                cy.wait(10 * SEC);
-                cy.get("h1").should("contain", applicationInventory);
+                // Temporary wait for MTA-3812; keep explicit timeout to reduce flakiness
+                cy.wait(10 * SEC);
+                cy.get("h1", { timeout: 60 * SEC }).should("contain", applicationInventory);

Optional (preferable longer-term): replace the fixed wait with an event-based wait (e.g., intercept the apps API or wait for a stable UI control like items-per-page toggle to be enabled) before asserting the header.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// Bug MTA-3812 Application Inventory page takes long to load
	// TODO: Wait of 10s to be reduced after above bug is fixed
	cy.wait(10 * SEC);
	cy.get("h1").should("contain", applicationInventory);
	// Bug MTA-3812 Application Inventory page takes long to load
	// TODO: Wait of 10s to be reduced after above bug is fixed
	// Temporary wait for MTA-3812; keep explicit timeout to reduce flakiness
	cy.wait(10 * SEC);
	cy.get("h1", { timeout: 60 * SEC }).should("contain", applicationInventory);

🤖 Prompt for AI Agents

In cypress/e2e/models/migration/applicationinventory/application.ts around lines
162 to 165, remove the flaky pattern of a fixed wait without an explicit
assertion timeout: keep the temporary cy.wait(10 * SEC) if MTA-3812 requires it,
but change the header assertion to include a longer explicit timeout (e.g.,
cy.get("h1", { timeout: 15000 }).should("contain", applicationInventory)) so the
check is resilient; optionally replace the fixed wait with an event-based wait
(intercept the apps API or wait for a specific UI control to become enabled) as
a longer-term improvement.

[abrugaro]

Is this PR still relevant?

I see the tests are passing in the pipelines without this being merged

Thanks!

[nachandr]

The after() hook from questionnaires_features.test.ts passed with this PR . See attached screenshot . So, this PR is still required. We are navigating to the Application Inventory page and deleting applications through the after() hook.

[nachandr]

The after() hook from questionnaires_features.test.ts failed without this PR . See attached screenshot.

[abrugaro]

Since the majority of the tests are passing without this explicit waiting I'd suggets to add it only to the specific test that is affected by the bug.

Otherwise, we are adding 10 seconds to a lot of tests so that will affect the total execution time of all tiers

[nachandr]

Since the majority of the tests are passing without this explicit waiting I'd suggets to add it only to the specific test that is affected by the bug.

Otherwise, we are adding 10 seconds to a lot of tests so that will affect the total execution time of all tiers

Hi @abrugaro , I'd like to clarify that the 1) explicit wait is being added only when forceReload is true, not every time the page loads and 2) the after hook() fails in other tests as well when the Application Inventory page is force reloaded.