Skip to content

fix: make Launching Soon modal reachable by adding trigger action#1104

Merged
SB2318 merged 9 commits into
SB2318:webfrom
Krishnx21:fix-unreachable-launching-soon-modal
Jun 6, 2026
Merged

fix: make Launching Soon modal reachable by adding trigger action#1104
SB2318 merged 9 commits into
SB2318:webfrom
Krishnx21:fix-unreachable-launching-soon-modal

Conversation

@Krishnx21
Copy link
Copy Markdown
Contributor

@Krishnx21 Krishnx21 commented Jun 4, 2026

Description

This PR fixes the issue where the "Launching Soon" modal could never be displayed because comingSoonModal was initialized as false and there was no code path that updated it to true.

Changes Made

  • Added a trigger to call setComingSoonModal(true) when users interact with the intended UI element.
  • Ensured the modal becomes visible when the associated feature/button/card is clicked.
  • Preserved the existing modal close behavior.
  • Verified that the modal opens and closes correctly without affecting other homepage functionality.

Problem

Previously:

const [comingSoonModal, setComingSoonModal] = useState(false);

The modal was conditionally rendered based on comingSoonModal, but the state was never updated to true, making the modal unreachable for users.

Solution

Implemented the missing trigger so the modal state is updated appropriately and the feature is now accessible.

Related Issue

Closes #1046

Testing

  • Verified the trigger opens the modal.
  • Verified the modal closes correctly.
  • Checked that no existing homepage functionality was affected.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

Thank you @, for creating the PR and contributing to our UltimateHealth project 💗.
Our team will review the PR and will reach out to you soon! 😇
Make sure that you have marked all the tasks that you are done with ✅.
Thank you for your patience! 😀

@SB2318
Copy link
Copy Markdown
Owner

SB2318 commented Jun 4, 2026

/review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

🤖 Gemini AI Code Review

Summary

This Pull Request aims to fix an issue where the "Launching Soon" modal was never displayed due to a missing trigger. The author has implemented a mechanism to show this modal when a specific UI element (the "Admin Panel" button for Android) is interacted with.

The PR successfully introduces the trigger for the modal and improves the semantic HTML for the button. However, it also introduces a significant functional change by replacing a direct link to the Google Play Store with the modal, which requires explicit confirmation of intent.

🔴 High Severity

  • Issue: The "UHealth Admin" button, previously an <a> tag linking directly to the Google Play Store (https://play.google.com/store/apps/details?id=com.ultimatehealth.admin), has been converted into a <button> that only triggers the "Launching Soon" modal.
  • Impact: This is a fundamental change in functionality. Users can no longer navigate directly to the UHealth Admin app on Google Play from this button. The PR description frames this as "making the modal reachable" and "implementing the missing trigger," but it doesn't explicitly state that an existing external navigation link is being removed and replaced by an internal modal. This could be an unintended side-effect or a significant functional change that needs explicit stakeholder approval. If the UHealth Admin app is already available on Google Play, removing the direct link could hinder user access.
  • Fix:
    1. Clarify Intent: Confirm with product owners/stakeholders whether the intention was to completely remove the direct link to the Google Play Store for the UHealth Admin app and replace it solely with the "Launching Soon" modal.
    2. If removal is intended: Update the PR description to clearly state that the direct link to the UHealth Admin app on Google Play has been removed and replaced with the "Launching Soon" modal.
    3. If removal is NOT intended: Revert the change from <a> to <button> for the UHealth Admin link. If the modal should still be shown, consider alternative UX patterns, such as:
      • Showing the modal before navigating, perhaps with a "Continue to Play Store" option.
      • Conditionally rendering either the <a> tag or the <button> based on a feature flag or the actual launch status of the UHealth Admin app.

🟡 Medium Severity

No medium severity issues identified.

🟢 Low Severity / Nits

  • Issue: The PageWrapper and Section components are imported in web/src/app/page.tsx but their usage is not visible in the provided diff.
  • Impact: Unused imports can clutter the codebase and, in some cases, slightly increase bundle size (though modern bundlers often tree-shake them out).
  • Fix: Please confirm if PageWrapper and Section are used elsewhere in web/src/app/page.tsx (outside the HeroAndDownload section shown in the diff). If not, they should be removed.

What's Good ✅

  1. Improved Accessibility and Semantic HTML: The change from an <a> tag to a <button> for an in-page action (triggering a modal) is a good practice for accessibility and semantic correctness. The updated aria-label is also descriptive and helpful.
  2. Clear Problem Statement: The PR description clearly articulates the problem (modal unreachable) and the proposed solution, making it easy to understand the core intent.
  3. Type Safety: The new onShowComingSoon prop is correctly typed in HeroAndDownloadProps, maintaining good type safety practices.

Verdict

Request Changes: The high-severity issue regarding the functional change of the "UHealth Admin" button requires immediate attention and clarification. This is a significant alteration to user flow that needs explicit confirmation before merging.

Copy link
Copy Markdown
Owner

@SB2318 SB2318 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the missing trigger for the "Launching Soon" modal.

However, I noticed that the existing "UHealth Admin" action appears to have been changed from a direct Google Play Store link to a button that only opens the modal.

Could you clarify whether this behavior change was intentional and approved?

From the previous implementation, users could directly navigate to the UHealth Admin Play Store page. With this PR, that navigation path seems to be removed entirely and replaced by the modal. Since the issue is about making the modal reachable, this introduces a broader functional change than the scope described in the PR.

If the app is genuinely not available yet, replacing the link may be appropriate. Otherwise, we should consider preserving the existing navigation behavior or documenting this change explicitly in the PR description.

Also, please confirm whether the removed Play Store link was intentionally deprecated.

@Krishnx21
Copy link
Copy Markdown
Contributor Author

@SB2318 thank you for reviewing i have updated it .

@SB2318 SB2318 removed the in-progress label Jun 5, 2026
Copy link
Copy Markdown
Owner

@SB2318 SB2318 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

@SB2318
Copy link
Copy Markdown
Owner

SB2318 commented Jun 5, 2026

/review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 5, 2026

🤖 Gemini AI Code Review

Summary

This PR aims to resolve an issue where the "Launching Soon" modal was inaccessible by introducing a trigger to update its state. The core change involves adding an onShowComingSoon prop to the HeroAndDownload component and its children, which is then invoked by a new "Launch Status" button.

While the primary intent of the PR is clear and the solution for the modal trigger appears functionally correct, the PR contains several significant unrelated changes and artifacts that severely impact its quality and reviewability.

🔴 High Severity

  • Issue: Unrelated Git Submodule Change
    The diff includes the addition/update of a Git submodule (ieee-submodules/IEEE-s-Mindful-Devs-Bootcamp). This is a substantial change that introduces an external dependency and affects the repository's structure and build process.

  • Impact: This change is completely unrelated to the PR's stated purpose of fixing the "Launching Soon" modal. It makes the PR difficult to review, potentially introduces new dependencies or build issues without proper justification, and violates the principle of single responsibility for PRs. Such a change should be in its own dedicated PR with a clear description and rationale.

  • Fix:
    Revert the submodule change from this PR. If the submodule update is necessary, it should be done in a separate PR.

  • Issue: Merge Conflict Markers in web/src/app/page.tsx
    The page.tsx file in the diff contains merge conflict markers (<<<<<<< fix-unreachable-launching-soon-modal, =======, >>>>>>> web).

  • Impact: These markers indicate an incomplete or improperly resolved merge. The code is in an invalid state and will cause compilation errors or unexpected behavior if merged as-is. It suggests the PR branch was not properly rebased or merged with the target branch.

  • Fix:
    Resolve the merge conflicts properly. The file should contain only the final, desired code without any conflict markers.

🟡 Medium Severity

  • Issue: Unjustified Removal of Layout Components in Login Pages
    The login-admin/page.tsx and login/page.tsx files have their imports for PageWrapper and Section removed, and the corresponding JSX elements are replaced with generic divs.
  • Impact: This change is unrelated to the "Launching Soon" modal fix. PageWrapper and Section components likely provide specific styling, layout, or accessibility features. Removing them without justification could lead to unintended UI regressions, layout shifts, or accessibility issues on the login pages. It also makes the PR harder to review by introducing unrelated changes.
  • Fix:
    Revert these changes. If PageWrapper and Section are truly redundant or causing issues, this should be addressed in a separate PR with a clear explanation and verification of no regressions.

🟢 Low Severity / Nits

  • Issue: Duplicate Import in web/src/app/page.tsx
    The page.tsx file has a duplicate import for ScrollToTop:
    --- a/web/src/app/page.tsx
    +++ b/web/src/app/page.tsx
    @@ -3,12 +3,15 @@
     import Image from "next/image";
     import "./globals.css";
     
    -import { type RefObject, useCallback, useEffect, useRef, useState, useSyncExternalStore } from "react";
     import { type RefObject, useCallback, useEffect, useRef, useState, useSyncExternalStore } from "react";
     import HeroAndDownload from "../components/HeroAndDownload";
     import ScrollToTop from "../components/ScrollToTop";
    + fix-unreachable-launching-soon-modal
    +import { PageWrapper, Section } from "../components/layout";
    +
     import ScrollToTop from "../components/ScrollToTop";
    + web
     import { Skeleton } from "../components/ui";
  • Impact: While not critical, duplicate imports are unnecessary, slightly increase bundle size, and indicate a lack of code hygiene.
  • Fix:
    Remove the duplicate import ScrollToTop from "../components/ScrollToTop";.

What's Good ✅

  1. Clear Problem Statement and Solution: The PR description clearly articulates the problem (unreachable modal) and the proposed solution (adding a trigger).
  2. Accessibility Focus: The new button includes a descriptive aria-label ("View UHealth Admin closed testing launch status"), which is excellent for screen reader users and overall accessibility.
  3. Type Safety with Pick: Using Pick<HeroAndDownloadProps, "onShowComingSoon"> for the AndroidStoreButtons and IosStoreButtons props is a good practice for ensuring type safety and only passing necessary props down the component tree.

Verdict

Request Changes: The presence of a high-severity, unrelated Git submodule change and unresolved merge conflicts makes this PR unmergeable and requires significant rework. The unrelated layout component removals also need to be addressed.

@SB2318
Copy link
Copy Markdown
Owner

SB2318 commented Jun 5, 2026

Post-Approval Review Findings

🔴 Blocking: Unresolved Merge Conflicts

web/src/app/page.tsx still contains merge conflict markers (<<<<<<<, =======, >>>>>>>).

This leaves the file in an invalid state and will prevent successful builds and merges. Please resolve the conflicts and ensure only the intended final code remains.


🔴 Blocking: Unrelated Submodule Modification

This PR includes an update/addition to the ieee-submodules/IEEE-s-Mindful-Devs-Bootcamp submodule.

The stated purpose of this PR is to make the "Launching Soon" modal reachable. The submodule change appears unrelated to that goal and significantly increases the review scope.

Please remove the submodule modification from this PR and submit it separately if it is required.


🟡 Needs Clarification: Login Layout Changes

The PR removes PageWrapper and Section from both login pages and replaces them with generic div containers.

These changes appear unrelated to the modal accessibility fix. If they are intentional, please explain the reason and confirm there are no styling, layout, or accessibility regressions. Otherwise, consider reverting them to keep the PR focused on a single concern.


🟢 Minor: Import Cleanup

There appears to be a duplicate ScrollToTop import in web/src/app/page.tsx.

This may be a byproduct of the unresolved merge conflict. After resolving conflicts, please verify imports are cleaned up and deduplicated.


✅ What's Good

  • The modal accessibility issue appears to be addressed through the new callback flow.
  • The added aria-label improves accessibility for screen reader users.
  • Using Pick<HeroAndDownloadProps, "onShowComingSoon"> keeps child component props focused and type-safe.
  • The solution follows the existing component hierarchy without introducing unnecessary complexity.

Summary

The primary feature implementation looks reasonable and appears to solve the original issue. However, the presence of unresolved merge conflicts and unrelated submodule modifications makes the PR unmergeable in its current state.

Please address the blocking issues and provide clarification for the login page layout changes before resubmission.

@Krishnx21
Copy link
Copy Markdown
Contributor Author

@SB2318 THANKS FOR REVIEWING
Addressed review feedback:

  • Removed unrelated submodule change from this PR.
  • Resolved merge-conflict residue and duplicate imports in web/src/app/page.tsx.
  • Restored login pages to use Section and PageWrapper.
  • Preserved the UHealth Admin Play Store link while adding a separate Launch Status modal trigger.

@SB2318 SB2318 removed the in-progress label Jun 6, 2026
@SB2318
Copy link
Copy Markdown
Owner

SB2318 commented Jun 6, 2026

/review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 6, 2026

🤖 Gemini AI Code Review

Summary

This Pull Request aims to resolve an issue where the "Launching Soon" modal was inaccessible to users by introducing a trigger to update its state. The core logic for enabling the modal via a new button and prop passing is sound and well-implemented.

However, the PR contains critical merge conflict markers in web/src/app/page.tsx which have been committed to the branch. This results in duplicate components being rendered and will lead to a broken UI and unexpected behavior. Additionally, there are unrelated refactoring changes in the login pages that should ideally be in a separate PR.

🔴 High Severity

  • Issue: Unresolved Merge Conflict Markers and Duplicate Component Rendering
    The file web/src/app/page.tsx contains merge conflict markers (<<<<<<<, =======, >>>>>>>) and consequently renders the HeroAndDownload component twice. This indicates that a merge conflict was not properly resolved before committing.

    + fix-unreachable-launching-soon-modal
    +      {/* ── Hero ── */}
    +      <HeroAndDownload
    +        onJoinTestFlight={() => setAppleModal(true)}
    +        onShowComingSoon={openComingSoonModal}
    +      />
    +
          {/* ── Hero + Downloads (new premium design) ── */}
          <HeroAndDownload onJoinTestFlight={() => setAppleModal(true)} />
    + web
  • Impact: This is a critical bug that will cause the application to render two HeroAndDownload sections on the homepage, leading to a severely broken UI and potentially incorrect functionality. The openComingSoonModal prop will only be passed to one of the duplicated components, making the new feature partially or completely non-functional as intended.

  • Fix:

    1. Resolve the merge conflict properly. This typically involves choosing the correct version of the code (likely the one with onShowComingSoon prop) and removing the conflict markers and the redundant HeroAndDownload component.
    2. Ensure only one HeroAndDownload component is rendered, and it receives all necessary props, including onShowComingSoon.
    --- a/web/src/app/page.tsx
    +++ b/web/src/app/page.tsx
    @@ -507,11 +507,9 @@ export default function Home() {
             </nav>
           </header>
     
    - fix-unreachable-launching-soon-modal
           {/* ── Hero ── */}
           <HeroAndDownload
             onJoinTestFlight={() => setAppleModal(true)}
             onShowComingSoon={openComingSoonModal}
           />
    -
    -      {/* ── Hero + Downloads (new premium design) ── */}
    -      <HeroAndDownload onJoinTestFlight={() => setAppleModal(true)} />
    - web
           {/* ── Screenshots ── */}
           <Section id="screenshots">
             <PageWrapper>

    (Assuming the intention was to update the existing HeroAndDownload with the new prop, not add a second one.)

🟡 Medium Severity

  • Issue: Unrelated Refactoring in Login Pages
    The PR includes changes in web/src/app/login-admin/page.tsx and web/src/app/login/page.tsx where div elements are replaced with Section and PageWrapper components. While this might be a semantic improvement, it is unrelated to the PR's stated purpose of fixing the "Launching Soon" modal.

  • Impact: Including unrelated changes makes the PR harder to review, increases the risk of introducing unintended side effects in areas not directly related to the feature, and complicates future debugging. It also deviates from the principle of single responsibility for PRs.

  • Fix: Revert these changes in this PR. Create a separate PR specifically for refactoring the layout components in the login pages. If these changes are absolutely critical for the modal functionality (which seems unlikely), they should be explicitly mentioned and justified in the PR description.

  • Issue: Unused Import withBasePath
    The withBasePath utility is imported in web/src/app/page.tsx but is not used anywhere in the file.

  • Impact: Unused imports can slightly increase bundle size (though Next.js often tree-shakes) and clutter the codebase, making it less readable and maintainable.

  • Fix: Remove the unused import from web/src/app/page.tsx.

    --- a/web/src/app/page.tsx
    +++ b/web/src/app/page.tsx
    @@ -7,9 +7,7 @@ import "./globals.css";
     import { type RefObject, useCallback, useEffect, useRef, useState, useSyncExternalStore } from "react";
     import HeroAndDownload from "../components/HeroAndDownload";
     import ScrollToTop from "../components/ScrollToTop";
    -
     import { PageWrapper, Section } from "../components/layout";
    -
    -import { withBasePath } from "@/lib/basePath";
     import { Skeleton } from "../components/ui";
     import PageWrapper from "../components/layout/PageWrapper";
     import Section from "../components/layout/Section";

🟢 Low Severity / Nits

  • Issue: Global CSS Import in Page Component
    The line import "./globals.css"; is present in web/src/app/page.tsx. In Next.js, global CSS files should ideally be imported only in _app.tsx (for Pages Router) or layout.tsx (for App Router) to ensure they apply globally without being re-imported on every page.

  • Impact: While it might work, importing global CSS in a page component can sometimes lead to unexpected styling issues or redundant imports if not handled carefully by the build system. It's generally a best practice to centralize global styles.

  • Fix: If globals.css contains truly global styles, move its import to the root layout.tsx or _app.tsx file. If it contains styles specific to the homepage, consider scoping them or renaming the file to reflect its local nature.

  • Issue: Minor Comment Formatting in basePath.ts
    The JSDoc comment for withBasePath has a minor formatting inconsistency (* vs *).

    --- a/web/src/lib/basePath.ts
    +++ b/web/src/lib/basePath.ts
    @@ -7,7 +7,7 @@ export const BASE_PATH = process.env.NEXT_PUBLIC_BASE_PATH || "";
     /**
      * Create a URL with the basePath prepended
      * Useful for internal navigation links
    - * 
    + *
      * @param path - The path relative to the app root (e.g., "/contribute")
      * @returns The path with basePath prepended (e.g., "/frontend/v2/contribute")
      */
  • Impact: Purely stylistic. No functional impact.

  • Fix: Adjust the comment to be consistent, e.g., * @param path instead of * @param path.

What's Good ✅

  1. Excellent Accessibility for New Button: The new "Launch Status - Closed Testing" button includes a descriptive aria-label="View UHealth Admin closed testing launch status". This is crucial for screen reader users and demonstrates a strong commitment to accessibility.
  2. Clear Prop Passing and Type Safety: The onShowComingSoon prop is clearly defined, passed down through components, and type-checked using Pick<HeroAndDownloadProps, "onShowComingSoon"> for AndroidStoreButtons and IosStoreButtons. This ensures type safety and makes the component API explicit and easy to understand.
  3. Problem Description and Solution Clarity: The PR description clearly outlines the problem (unreachable modal), the solution (adding a trigger), and the changes made. This is very helpful for understanding the intent.

Verdict

Request Changes

The presence of critical merge conflict markers and duplicate components in web/src/app/page.tsx is a high-severity issue that must be resolved before this PR can be merged. Additionally, the unrelated refactoring in the login pages should be addressed.

@SB2318 SB2318 merged commit af93bbf into SB2318:web Jun 6, 2026
4 checks passed
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 6, 2026

Congratulations, Your pull request has been successfully merged 🥳🎉 Thank you for your contribution to the project 🚀 Keep Contributing!! ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants