Skip to content

feat(CHIM-620): migrate navigation and routing to IonReactHashRouter#4693

Open
manish7321 wants to merge 6 commits into
devfrom
feature/CHIM-620-hash-router-navigation
Open

feat(CHIM-620): migrate navigation and routing to IonReactHashRouter#4693
manish7321 wants to merge 6 commits into
devfrom
feature/CHIM-620-hash-router-navigation

Conversation

@manish7321

@manish7321 manish7321 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

What

Migrates app navigation from BrowserRouter/IonReactRouter assumptions to IonReactHashRouter and updates related routing behavior across the app.

This includes:

  • moving app routing/content setup into AppContent
  • adding hash-aware router location helpers for pathname, search, hash, and URL replacement
  • updating history.push / history.replace calls that pass route state so they work correctly with hash routing
  • fixing popup navigation and GrowthBook generic popup handling
  • updating analytics, logging, auth redirects, and reload flows to read app routes correctly under hash routing
  • updating affected tests and router mocks to match the new routing model

Why

The app was being migrated to Ionic hash-based routing, but many flows still assumed browser-style paths and legacy stateful navigation
behavior.

That caused risk of broken or inconsistent:

  • route state passing
  • popup CTA navigation
  • analytics/logging page tracking
  • auth redirect behavior
  • reload / resume flows
  • unit tests that still mocked the old router setup

How

  • Replaced the app router setup with IonReactHashRouter and removed the extra top-level BrowserRouter.
  • Extracted route/content wiring into AppContent to keep bootstrap simpler while preserving existing overlays, route effects, and hardware back handling.
  • Added src/utility/routerLocation.ts to centralize app-aware URL/path handling for both hash and non-hash routing cases.
  • Converted stateful navigation calls from history.push(path, state) / history.replace(path, state) to location objects built with parsePath(...) and state, so route state is preserved reliably.
  • Updated popup navigation to build replacement URLs correctly for hash routing and fixed popup lifecycle handling by clearing popup-active state before navigation.
  • Switched generic popup feature reads to useFeatureValue, broadened popup config typing, and aligned related tests/mocks.
  • Preserved important local storage keys during data clearing and updated OAuth redirect handling to respect the app base path.

Testing

  • Unit tests
    • Updated router-affected tests and mocks, including App, Home, LiveQuizGame, generic popup navigation/manager, ProfileMenu, StickerBook,
      QRAssignments, and TeacherRecommendedAssignments.

Risks

  • Navigation regressions may still exist in less-used flows that depend on route state, query params, or direct window.location assumptions.
  • Web login redirect, deep link, popup CTA, and native reload/resume flows are the highest-risk areas because this PR changes URL construction and replacement behavior.
  • Broad routing changes touch student, teacher, and ops-console flows, so smoke testing on both web and Android is recommended.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the application's routing to support hash-based routing by replacing IonReactRouter and BrowserRouter with IonReactHashRouter, introducing a centralized routerLocation utility to handle pathnames and search parameters, and updating history navigation calls to use parsePath. Feedback on these changes highlights a missing React import in AppContent.tsx, a potential invalid URL construction when appending query parameters in LiveQuizGame.tsx, and an unsafe iteration over localStorage during deletion in DataClear.tsx.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/app/AppContent.tsx
@@ -0,0 +1,53 @@
import { IonRouterOutlet } from '@ionic/react';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The newly added AppContent component uses React.FC but does not import React. This will cause a TypeScript compilation error (Cannot find name 'React'). Please import React at the top of the file.

import React from 'react';\nimport { IonRouterOutlet } from '@ionic/react';

Comment on lines +143 to +146
history.replace({
...parsePath(fromPath + '&isReload=false'),
state: returnState,
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Blindly appending &isReload=false to fromPath can result in an invalid URL path (e.g., /home&isReload=false) if fromPath does not already contain query parameters.\n\nPlease check if fromPath contains ? and use the appropriate separator (? or &) to construct the path, similar to how it is handled in LidoPlayer.tsx.

      const separator = fromPath.includes('?') ? '&' : '?';\n      history.replace({\n        ...parsePath("${fromPath}${separator}isReload=false"),\n        state: returnState,\n      });

Comment on lines +37 to +42
for (let i = localStorage.length - 1; i >= 0; i--) {
const key = localStorage.key(i);
if (key && !KEYS_TO_PRESERVE.has(key)) {
localStorage.removeItem(key);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Iterating backwards over localStorage while calling removeItem can lead to skipped keys or unexpected behavior. According to the HTML Storage specification, the order of keys is implementation-defined and only guaranteed to be consistent as long as the number of keys does not change. Mutating the storage during iteration changes the key indices dynamically.\n\nTo ensure robust and safe deletion, collect all keys into an array first, and then iterate over that array to remove the non-preserved keys.

    const keys: string[] = [];\n    for (let i = 0; i < localStorage.length; i++) {\n      const key = localStorage.key(i);\n      if (key) {\n        keys.push(key);\n      }\n    }\n    for (const key of keys) {\n      if (!KEYS_TO_PRESERVE.has(key)) {\n        localStorage.removeItem(key);\n      }\n    }

@manish7321
manish7321 force-pushed the feature/CHIM-620-hash-router-navigation branch from f8ec50f to db03d07 Compare July 10, 2026 11:54
Move app routing/content into AppContent and simplify App bootstrap. Switch from the legacy GrowthBook API to useFeatureValue across pages/hooks to read the GENERIC_POP_UP feature. Add URL building for hash vs non-hash routers and update replaceLocation to respect app routing. Fix GenericPopUp behavior (isPopupActive lifecycle) and broaden PopupConfig typing to include JSONValue. Clean up DataClear and Sidebar localStorage handling. Update tests and mocks to match router and GrowthBook changes.

Files changed: App.tsx, app/AppContent.tsx, hooks/useGenericPopup.ts, pages/*, components/GenericPopUp/*, navigation utilities, tests and mocks.

Why: reduces bootstrap complexity, unifies feature-flag usage, fixes popup navigation issues, and aligns tests with hash routing.
@manish7321
manish7321 force-pushed the feature/CHIM-620-hash-router-navigation branch from ce81256 to d3073d6 Compare July 14, 2026 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants