OpenConceptLab/ocl_online#374 | using path routing in place of hash routing by snyaggarwal · Pull Request #61 · OpenConceptLab/oclweb3

snyaggarwal · 2026-10-05T14:56:20Z

Linked Issue

Refs OpenConceptLab/ocl_online#374

…outing

paynejd

Reviewed against OpenConceptLab/ocl_online#374, and ran it locally against prod data: old /#/ links redirect (including ?utm_…#/…), in-app clicks and Back don't reload, and dotted concept URLs refresh. The inline suggestions were checked in that build, and lint passes with them. Codex's review follows.

Before merge

Forced logout loses the page's query after its first parameter (utils.js L681).
history.js should accept only single-slash routes (L13, L31). Details in OpenConceptLab/ocl_online#374.

Small
3. ?origin=openmrs now redirects from every path (index.html L47).
4. A relative #/… link picks up the current page's query (history.js L61).
5. Still /#: toFullURL ("view in source" on concepts and mappings) and AddReferencesDialog.jsx:313: drop the #. toMapperURL can keep it until both apps are live.

Two of Codex's points don't happen today: no OCL app puts referrer before the # (all put it inside the hash), and every blob: link has download.

paynejd · 2026-10-06T16:00:30Z

  const redirectURL = forced ?
-    window.location.origin + '/#/signin?returnTo=' + encodeURIComponent(returnTo) :
+    window.location.origin + '/signin?returnTo=' + encodeURIComponent(returnTo) :


oclapi2 decodes post_logout_redirect_uri and passes it to Keycloak raw, so Keycloak splits it at each &: /search/?q=x&type=concepts comes back as /search/?q=x, even if getSSOLogoutURL encodes it (Codex's fix). As a fragment, the pre-PR form, it never reaches oclapi2 or Keycloak, so their redirects carry it through intact; the legacy-hash redirect then turns it into /signin?returnTo=…. Checked against a mock of both redirects. Keycloak's post-logout URIs then need no change.

Suggested change

const redirectURL = forced ?

window.location.origin + '/#/signin?returnTo=' + encodeURIComponent(returnTo) :

window.location.origin + '/signin?returnTo=' + encodeURIComponent(returnTo) :

// A fragment never reaches oclapi2 or Keycloak, so it survives their redirects intact (a query is decoded and

// split on '&'). Back in the app, the legacy-hash redirect turns it into /signin?returnTo=…

const redirectURL = forced ?

window.location.origin + '/#/signin?returnTo=' + encodeURIComponent(returnTo) :

paynejd · 2026-10-06T16:00:31Z

+  if(!path)
+    return
+  const routePath = toRoutePath(path)


Same-site paths only. Details in OpenConceptLab/ocl_online#374.

Suggested change

if(!path)

return

const routePath = toRoutePath(path)

const routePath = toRoutePath(path)

// Same-site paths only: '//host' or '/\host' would leave the site, or make pushState throw.

if(!/^\/(?![/\\])/.test(routePath || ''))

return

paynejd · 2026-10-06T16:00:31Z

+
+export const legacyHashRoute = (location=window.location) => {
+  const { hash, search } = location
+  if(!hash.startsWith('#/') || /[?&]referrer=/.test(search))


Skip #//… and #/\…. Details in OpenConceptLab/ocl_online#374.

Suggested change

if(!hash.startsWith('#/') || /[?&]referrer=/.test(search))

if(!hash.startsWith('#/') || /^#\/[/\\]/.test(hash) || /[?&]referrer=/.test(search))

paynejd · 2026-10-06T16:00:31Z

+  const url = new URL(anchor.href, window.location.href)
+  if(url.origin !== window.location.origin)


A relative #/… href resolves against this page, so legacyHashRoute merges this page's query into the route: on /search/?q=malaria&type=concepts, #/orgs/CIEL/ goes to /orgs/CIEL/?q=malaria&type=concepts. Also limits routing to http(s) links.

Suggested change

const url = new URL(anchor.href, window.location.href)

if(url.origin !== window.location.origin)

// Resolved against this page, a relative '#/…' link would pick up this page's query.

if(href.startsWith('#/'))

return href.slice(1)

const url = new URL(anchor.href, window.location.href)

if(url.origin !== window.location.origin || !/^https?:$/.test(url.protocol))

paynejd · 2026-10-06T16:00:31Z

+         go('/oidc/login/' + loc.search + '&next=' + loc.pathname);
+       } else if(loc.href.includes('/?session_state=')) {
+         go('/');
+       } else if((loc.search || '').includes('origin=openmrs')) {


Under hash routing only / (and /search) URLs carried origin=openmrs outside the hash. Now /orgs/CIEL/sources/CIEL/?origin=openmrs lands on the org search.

Suggested change

} else if((loc.search || '').includes('origin=openmrs')) {

} else if((loc.pathname === '/' || loc.pathname.startsWith('/search')) && (loc.search || '').includes('origin=openmrs')) {

paynejd

Codex adversarial review, pass 1 (codex-cli 0.160.1, commit a01e34d)

I found five correctness issues and one incomplete migration. No files were modified.

[P1] Forced logout corrupts the return URL’s query string.
src/common/utils.js:682 encodes returnTo, but getSSOLogoutURL interpolates the resulting URL into post_logout_redirect_uri without encoding that outer value.

Reproduced: after ordinary query decoding at both boundaries, /search/?q=a%26b&type=concepts becomes /search/?q=a&b. The search term changes and type disappears. Actual Keycloak behavior was not exercised.

Fix: construct the logout query with URLSearchParams, encoding the complete post_logout_redirect_uri as one parameter.
[P1] Legacy URLs containing an outer referrer parameter lose their destination.
src/common/history.js:31 skips conversion whenever the outer query contains referrer.

Reproduced: /?referrer=https%3A%2F%2Fopenconceptlab.org%2F#/orgs/CIEL/sources/CIEL/ returns no legacy route. BrowserRouter consequently sees /; authenticated startup also removes the hash, leaving the dashboard.

Fix: distinguish actual legacy routing fragments from fragments embedded in an unencoded referrer. An encoded referrer should not prevent conversion of a real legacy route.
[P2] Legacy-route validation. Moved to OpenConceptLab/ocl_online#374 (private).
[P2] The click interceptor treats every same-origin resource as an app route.
src/common/history.js:61 checks origin without checking protocol or whether the destination belongs to the SPA.

Reproduced: ordinary clicks on /env-config.js, /bootstrap.min.css, and same-origin blob: URLs are prevented and sent to router history. A resource link in About/Markdown content consequently cannot open its resource normally.

Fix: intercept only HTTP(S) destinations covered by app routes, with native navigation retained for resources and other destinations.
[P2] Real document fragments are discarded or fail to scroll.
src/components/app/App.jsx:131 removes every hash on authenticated startup outside the callback route. A deep link ending in #license loses its fragment, including one preserved by legacy conversion.

Separately, the global handler prevents native navigation for a pathname-qualified fragment link and pushes history without scrolling to its target.

Verified: fragment removal and interception are present in the code; actual rendered scrolling was not tested.

Fix: preserve document fragments, remove only recognized routing/referrer fragments, and handle scrolling after destination content renders.
[P3] Several producers still emit hash URLs.
The complete src/ and public/ search found:
- src/common/utils.js:124: toFullURL, still used by concept/mapping “view in source” actions.
- The same file at line 1135: toMapperURL, used by the Mapper menu link.
- src/components/collections/AddReferencesDialog.jsx:313: “open repository” still constructs /#….
- The sibling PR’s toV3URL likewise still emits /#….
Fix: update these producers to path URLs. Keep toV2URL’s /# and legacy-input parsing, as required.

Verification completed:

Ran 31 in-memory Node checks against the history helpers and extracted authentication/bootstrap code. These confirmed ordinary legacy query/fragment preservation, dotted IDs, and skipping modifier clicks, middle clicks, named targets, _blank, downloads, and external links.
Reviewed Dockerfile, startup script, webpack, and nginx. Absolute bundle paths and the existing try_files $uri /index.html support deep paths containing dots. The production image was not executed.
Compared main since the merge base: three commits changed versioning, copied locale external IDs, and feed-error handling. They introduce no new hash links or apparent routing conflicts.
node_modules is absent; there are no test files or test script, and the PR adds no helper tests. git diff --check passed; the worktree remains clean.

Analytics remains unverified: the manual page-view call runs at App mount, and setUpRecentHistory has no callers. GA4 enhanced measurement may cover history transitions, but its configuration and emitted events need checking in DebugView. Google’s SPA guidance

Full browser back/forward behavior, React/MUI propagation, Keycloak sign-in/signup/logout, email verification/reset, and production serving remain untested because browser tooling was unavailable and dependencies were absent.

Overall verdict: merge after fixes.

OpenConceptLab/ocl_online#374 | using path routing in place of hash r…

a01e34d

…outing

snyaggarwal requested a review from paynejd October 5, 2026 14:56

snyaggarwal self-assigned this Oct 5, 2026

snyaggarwal mentioned this pull request Oct 5, 2026

OpenConceptLab/ocl_online#374 | using path routing in place of hash routing OpenConceptLab/oclmap#91

Open

paynejd reviewed Oct 6, 2026

View reviewed changes

Navigation Menu

Repository navigation

OpenConceptLab/ocl_online#374 | using path routing in place of hash routing - #61

OpenConceptLab/ocl_online#374 | using path routing in place of hash routing#61
snyaggarwal wants to merge 1 commit into
mainfrom
ocl_online#374

snyaggarwal commented Oct 5, 2026 •

edited

Loading

Uh oh!

paynejd left a comment •

edited

Loading

Uh oh!

paynejd Oct 6, 2026

Uh oh!

paynejd Oct 6, 2026

Uh oh!

paynejd Oct 6, 2026

Uh oh!

paynejd Oct 6, 2026

Uh oh!

paynejd Oct 6, 2026

Uh oh!

paynejd left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	if(!hash.startsWith('#/') \|\| /[?&]referrer=/.test(search))
	if(!hash.startsWith('#/') \|\| /^#\/[/\\]/.test(hash) \|\| /[?&]referrer=/.test(search))

		const url = new URL(anchor.href, window.location.href)
		if(url.origin !== window.location.origin)

-  const url = new URL(anchor.href, window.location.href)
-  if(url.origin !== window.location.origin)
+  // Resolved against this page, a relative '#/…' link would pick up this page's query.
+  if(href.startsWith('#/'))
+    return href.slice(1)
+  const url = new URL(anchor.href, window.location.href)
+  if(url.origin !== window.location.origin || !/^https?:$/.test(url.protocol))

	} else if((loc.search \|\| '').includes('origin=openmrs')) {
	} else if((loc.pathname === '/' \|\| loc.pathname.startsWith('/search')) && (loc.search \|\| '').includes('origin=openmrs')) {

Conversation

snyaggarwal commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Linked Issue

Uh oh!

paynejd left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paynejd Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

paynejd Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

paynejd Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

paynejd Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

paynejd Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

paynejd left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

snyaggarwal commented Oct 5, 2026 •

edited

Loading

paynejd left a comment •

edited

Loading