fix(app): support runtime base paths for subpath deployments - #5969
Open
nutzlastfan wants to merge 4 commits into
Open
fix(app): support runtime base paths for subpath deployments#5969nutzlastfan wants to merge 4 commits into
nutzlastfan wants to merge 4 commits into
Conversation
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
nutzlastfan
marked this pull request as ready for review
April 20, 2026 06:41
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe PR adds runtime base path resolution to enable flexible app deployment. Core utilities normalize public URLs, infer paths from ChangesRuntime Base Path Resolution
Sequence Diagram(s)sequenceDiagram
participant index.js as App Entry
participant setRuntimePublicPath as Runtime Bootstrap
participant Window as window globals
participant Webpack as __webpack_public_path__
index.js->>setRuntimePublicPath: import (side-effect)
setRuntimePublicPath->>setRuntimePublicPath: resolve base path from window
setRuntimePublicPath->>Window: assign __OHIF_BASE_PATH__
setRuntimePublicPath->>Window: assign PUBLIC_URL
setRuntimePublicPath->>Window: initialize config.routerBasename
setRuntimePublicPath->>Webpack: set asset loading path
setRuntimePublicPath->>index.js: bootstrap complete
index.js->>index.js: continue with config load and React render
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform/app/public/html-templates/rollbar.html`:
- Around line 63-88: The getLocationBasePathFromPathname function currently
finds viewerIndex via lastIndexOf('/viewer') which matches substrings inside
segment names (e.g. '/viewer-prod' or '/viewers/123'); change the detection to
require a full path segment boundary (match '/viewer' followed by '/' or
end-of-string) when computing viewerIndex, then use that bounded index to call
normalizePublicUrl as before; update the logic in
getLocationBasePathFromPathname (and keep it in sync with
platform/core/src/utils/publicUrl.ts) so only the exact '/viewer' segment is
treated as the viewer route.
In `@platform/core/src/utils/publicUrl.ts`:
- Around line 39-43: The code uses lowerPath.lastIndexOf('/viewer')
(viewerIndex) and then truncates using locationPath.substring, which incorrectly
matches prefixes inside longer segments like '/viewers'; update the check to
only accept '/viewer' as a distinct path segment by verifying the character
immediately after the match is either absent (end of string) or a '/' before
returning normalizePublicUrl(locationPath.substring(0, viewerIndex)); apply the
identical boundary-safe check to the duplicated occurrences in
platform/app/public/html-templates/index.html and
platform/app/public/html-templates/rollbar.html so
getLocationBasePathFromPathname no longer truncates on '/viewers' or other
longer segment names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b6798276-46e8-4d72-8246-05911bc2139c
⛔ Files ignored due to path filters (2)
platform/app/.webpack/webpack.pwa.jsis excluded by!**/.webpack/**platform/app/.webpack/writePluginImportsFile.jsis excluded by!**/.webpack/**
📒 Files selected for processing (9)
platform/app/public/html-templates/index.htmlplatform/app/public/html-templates/rollbar.htmlplatform/app/public/manifest.jsonplatform/app/src/index.jsplatform/app/src/setRuntimePublicPath.jsplatform/app/src/utils/publicUrl.tsplatform/core/src/index.tsplatform/core/src/utils/index.tsplatform/core/src/utils/publicUrl.ts
Comment on lines
+63
to
+88
| function getLocationBasePathFromPathname(pathname) { | ||
| var locationPath = typeof pathname === 'string' && pathname.length > 0 ? pathname : '/'; | ||
| var lowerPath = locationPath.toLowerCase(); | ||
| var viewerIndex = lowerPath.lastIndexOf('/viewer'); | ||
|
|
||
| if (viewerIndex >= 0) { | ||
| return normalizePublicUrl(locationPath.substring(0, viewerIndex)); | ||
| } | ||
|
|
||
| if (lowerPath.endsWith('/index.html')) { | ||
| return normalizePublicUrl( | ||
| locationPath.substring(0, locationPath.length - '/index.html'.length) | ||
| ); | ||
| } | ||
|
|
||
| if (locationPath === '/') { | ||
| return '/'; | ||
| } | ||
|
|
||
| var lastSegment = locationPath.substring(locationPath.lastIndexOf('/') + 1); | ||
| if (lastSegment.indexOf('.') !== -1) { | ||
| return normalizePublicUrl(locationPath.substring(0, locationPath.lastIndexOf('/'))); | ||
| } | ||
|
|
||
| return normalizePublicUrl(locationPath); | ||
| } |
There was a problem hiding this comment.
/viewer is matched as a substring, not a path segment.
lastIndexOf('/viewer') matches longer segments too (e.g. /viewers/123 or a deployment prefix like /viewer-prod/), causing the base path to be truncated incorrectly. For /viewer-prod/ this yields / instead of /viewer-prod/, breaking asset resolution for that deployment. Constrain the match to a full segment boundary. The same heuristic lives in platform/core/src/utils/publicUrl.ts; keep both in sync.
🐛 Proposed fix
var viewerIndex = lowerPath.lastIndexOf('/viewer');
- if (viewerIndex >= 0) {
+ var afterViewer = lowerPath.charAt(viewerIndex + '/viewer'.length);
+ if (viewerIndex >= 0 && (afterViewer === '' || afterViewer === '/')) {
return normalizePublicUrl(locationPath.substring(0, viewerIndex));
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/app/public/html-templates/rollbar.html` around lines 63 - 88, The
getLocationBasePathFromPathname function currently finds viewerIndex via
lastIndexOf('/viewer') which matches substrings inside segment names (e.g.
'/viewer-prod' or '/viewers/123'); change the detection to require a full path
segment boundary (match '/viewer' followed by '/' or end-of-string) when
computing viewerIndex, then use that bounded index to call normalizePublicUrl as
before; update the logic in getLocationBasePathFromPathname (and keep it in sync
with platform/core/src/utils/publicUrl.ts) so only the exact '/viewer' segment
is treated as the viewer route.
Comment on lines
+39
to
+43
| const viewerIndex = lowerPath.lastIndexOf('/viewer'); | ||
|
|
||
| if (viewerIndex >= 0) { | ||
| return normalizePublicUrl(locationPath.substring(0, viewerIndex)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C2 "path:\s*['\"\`].*viewer" -g '!**/*.test.*'Repository: OHIF/Viewers
Length of output: 393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the exact implementation in publicUrl.ts
echo "---- publicUrl.ts (lines around viewer match) ----"
sed -n '1,120p' platform/core/src/utils/publicUrl.ts | nl -ba | sed -n '30,90p'
# 2) Inspect the duplicated logic in index.html template
echo "---- index.html template (viewer match) ----"
# Print around the line range mentioned in the review comment; fallback to finding the pattern if lines shifted.
sed -n '1,180p' platform/app/public/html-templates/index.html | nl -ba | sed -n '50,130p'
echo "---- find occurrences of lastIndexOf('/viewer') in repo ----"
rg -n "lastIndexOf\('/viewer'\)" -S . || true
echo "---- find occurrences of '/viewer' usage in routing ----"
# Look for path definitions and also raw '/viewer' strings.
rg -n --type=ts --type=tsx -g '!**/*.test.*' "path\s*:\s*['\"`].*\/viewer" . || true
rg -n --type=ts --type=tsx -g '!**/*.test.*' "['\"`]\/viewer['\"`]" . || true
echo "---- find how 'viewer' is used in route paths (broader) ----"
rg -n --type=ts --type=tsx -g '!**/*.test.*' "path\s*:\s*['\"`].*viewer" . || trueRepository: OHIF/Viewers
Length of output: 168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- publicUrl.ts (lines around viewer match) ----"
cat -n platform/core/src/utils/publicUrl.ts | sed -n '25,80p'
echo "---- index.html template (viewer match) ----"
cat -n platform/app/public/html-templates/index.html | sed -n '40,140p'
echo "---- find occurrences of lastIndexOf('/viewer') in repo ----"
rg -n "lastIndexOf\('/viewer'\)" -S . || trueRepository: OHIF/Viewers
Length of output: 6508
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- find occurrences of '/viewer' in path/router config (TS/TSX only) ----"
rg -n --type=ts --type=tsx -g '!**/*.test.*' "path\s*:\s*['\"`].*\/viewer" . || true
echo "---- find literal '/viewer' strings (TS/TSX only) ----"
rg -n --type=ts --type=tsx -g '!**/*.test.*' "['\"`]/viewer['\"`]" . || trueRepository: OHIF/Viewers
Length of output: 219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- rollbar.html (viewerIndex logic) ----"
cat -n platform/app/public/html-templates/rollbar.html | sed -n '50,95p'
echo "---- TS/TSX: literal \"/viewer\" occurrences (likely route strings) ----"
rg -n --type=ts --type=tsx -g '!**/*.test.*' "/viewer" -S . || true
echo "---- TS/TSX: literal \"/viewers\" occurrences ----"
rg -n --type=ts --type=tsx -g '!**/*.test.*' "/viewers" -S . || true
echo "---- TS/TSX: literal \"viewer-\" occurrences ----"
rg -n --type=ts --type=tsx -g '!**/*.test.*' "viewer-" -S . || trueRepository: OHIF/Viewers
Length of output: 2190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Also check non-TS templates/config for /viewer segment usage.
echo "---- non-TS: occurrences of \"/viewer\" (templates/config/etc) ----"
rg -n --glob '!**/*.ts' --glob '!**/*.tsx' -g '!**/*.test.*' "/viewer" -S . || trueRepository: OHIF/Viewers
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- usages of getLocationBasePathFromPathname ----"
rg -n --glob '*.ts' --glob '*.tsx' -g '!**/*.test.*' "getLocationBasePathFromPathname" . || true
echo "---- usages of resolveRuntimeBasePathFromWindow / __OHIF_BASE_PATH__ ----"
rg -n --glob '*.ts' --glob '*.tsx' -g '!**/*.test.*' "__OHIF_BASE_PATH__|resolveRuntimeBasePathFromWindow" . || true
echo "---- literal \"/viewer\" occurrences in TS/TSX ----"
rg -n --glob '*.ts' --glob '*.tsx' -g '!**/*.test.*' "\"/viewer\"|'/viewer'|`/viewer`|/viewer" . || true
echo "---- literal \"/viewers\" occurrences in TS/TSX ----"
rg -n --glob '*.ts' --glob '*.tsx' -g '!**/*.test.*' "\"/viewers\"|'/viewers'|`/viewers`|/viewers" . || true
echo "---- literal \"viewer\" route segment patterns (path: '...viewer...') in TS/TSX (limited output) ----"
rg -n --glob '*.ts' --glob '*.tsx' -g '!**/*.test.*' "path\s*:\s*['\"`].*viewer" . | head -n 50 || trueRepository: OHIF/Viewers
Length of output: 50570
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- occurrences of lastIndexOf('/viewer') in templates/html ----"
rg -n "lastIndexOf\('/viewer'\)" platform/app/public/html-templates -S || true
echo "---- viewerIndex logic context in rollbar.html ----"
cat -n platform/app/public/html-templates/rollbar.html | sed -n '55,90p'Repository: OHIF/Viewers
Length of output: 1820
🏁 Script executed:
#!/bin/bash
set -euo pipefail
RG_BASE="platform"
echo "---- router/path definitions containing viewer (ts/tsx) ----"
rg -n --glob '*.ts' --glob '*.tsx' -g '!**/*.test.*' \
"path\s*:\s*['\"].*viewer" "$RG_BASE" -S --max-count 200 || true
echo "---- literal /viewer and /viewers in platform (ts/tsx) ----"
rg -n --glob '*.ts' --glob '*.tsx' -g '!**/*.test.*' \
"/viewer|/viewers" "$RG_BASE" -S --max-count 200 || true
echo "---- literal viewer- in platform (ts/tsx) ----"
rg -n --glob '*.ts' --glob '*.tsx' -g '!**/*.test.*' \
"viewer-" "$RG_BASE" -S --max-count 200 || true
echo "---- any literal '/viewer' in non-code templates/static config (excluding node_modules) ----"
rg -n --glob '*.html' --glob '*.js' --glob '*.json' --glob '*.ts' --glob '*.tsx' -g '!**/node_modules/**' \
"['\"]/viewer['\"]" "$RG_BASE" -S --max-count 200 || trueRepository: OHIF/Viewers
Length of output: 1263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Route-like strings in configs (excluding tests)
echo "---- path: 'viewer*' (route-like) ----"
rg -n --glob '*.ts' --glob '*.tsx' -g '!**/*.test.*' \
"path\s*:\s*['\"]viewer[^'\"]*['\"]" . -S --max-count 200 || true
echo "---- modePath: 'viewer*' ----"
rg -n --glob '*.ts' --glob '*.tsx' -g '!**/*.test.*' \
"modePath\s*:\s*['\"]viewer[^'\"]*['\"]" . -S --max-count 200 || true
echo "---- route basename 'viewer*' in any string ----"
rg -n --glob '*.ts' --glob '*.tsx' -g '!**/*.test.*' \
"['\"][^'\"]*\/viewer[^'\"]*['\"]" . -S --max-count 200 || true
echo "---- any literal '/viewers' (route-like) ----"
rg -n --glob '*.ts' --glob '*.tsx' -g '!**/*.test.*' \
"/viewers" . -S --max-count 200 || trueRepository: OHIF/Viewers
Length of output: 992
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- literal '/viewers' across repo (exclude node_modules) ----"
rg -n "/viewers" -S . -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' -g '!**/.next/**' --max-count 200 || true
echo "---- literal '/viewer-' across repo (exclude node_modules) ----"
rg -n "/viewer-" -S . -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' -g '!**/.next/**' --max-count 200 || true
echo "---- literal '/viewer' across repo (exclude node_modules) ----"
rg -n "/viewer" -S . -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' -g '!**/.next/**' --max-count 200 || trueRepository: OHIF/Viewers
Length of output: 50370
Fix segment matching in getLocationBasePathFromPathname (avoid truncating on /viewers etc.)
lowerPath.lastIndexOf('/viewer') is substring-based: it will match the /viewer prefix inside longer segments like /viewers/..., truncating the computed base path incorrectly. Apply the same boundary-safe fix in platform/core/src/utils/publicUrl.ts and both duplicated copies in:
platform/app/public/html-templates/index.htmlplatform/app/public/html-templates/rollbar.html
Current route/path strings found in code include viewer/dicomlocal and viewer-cs3d, but boundary-safe matching avoids fragile dependence on “no other viewer* segment names” staying true.
🐛 Proposed segment-boundary check
- const viewerIndex = lowerPath.lastIndexOf('/viewer');
-
- if (viewerIndex >= 0) {
- return normalizePublicUrl(locationPath.substring(0, viewerIndex));
- }
+ const viewerMatch = lowerPath.match(/\/viewer(?=$|[/?#])/);
+ if (viewerMatch && viewerMatch.index !== undefined) {
+ return normalizePublicUrl(locationPath.substring(0, viewerMatch.index));
+ }📝 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
| const viewerIndex = lowerPath.lastIndexOf('/viewer'); | |
| if (viewerIndex >= 0) { | |
| return normalizePublicUrl(locationPath.substring(0, viewerIndex)); | |
| } | |
| const viewerMatch = lowerPath.match(/\/viewer(?=$|[/?#])/); | |
| if (viewerMatch && viewerMatch.index !== undefined) { | |
| return normalizePublicUrl(locationPath.substring(0, viewerMatch.index)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/core/src/utils/publicUrl.ts` around lines 39 - 43, The code uses
lowerPath.lastIndexOf('/viewer') (viewerIndex) and then truncates using
locationPath.substring, which incorrectly matches prefixes inside longer
segments like '/viewers'; update the check to only accept '/viewer' as a
distinct path segment by verifying the character immediately after the match is
either absent (end of string) or a '/' before returning
normalizePublicUrl(locationPath.substring(0, viewerIndex)); apply the identical
boundary-safe check to the duplicated occurrences in
platform/app/public/html-templates/index.html and
platform/app/public/html-templates/rollbar.html so
getLocationBasePathFromPathname no longer truncates on '/viewers' or other
longer segment names.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
This ports the runtime base-path work from a downstream OHIF 3.11 deployment into current upstream
master.The main issue is that a single build still assumes a fixed build-time
PUBLIC_URL, which breaks chunk loading, dynamic extension imports, and PWA asset resolution when the viewer is mounted under a subpath instead of/.Changes
@ohif/coreResult
A single viewer build can resolve static assets and dynamic imports correctly when served from paths like
/some-prefix/viewer/...instead of only from the site root.Notes
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR introduces runtime base-path resolution so a single OHIF build can be served from any subpath (e.g.
/some-prefix/viewer/) without a rebuild. It adds canonical helpers in@ohif/core, injects an inline<script>that sets<base href>before any head resources load, sets__webpack_public_path__from the resolved path at bundle entry time, and makes manifest/chunk URLs relative.platform/core/src/utils/publicUrl.ts— new canonical module providingnormalizePublicUrl,toRouterBasename,getLocationBasePathFromPathname,resolveRuntimeBasePathFromWindow, andgetPublicSubPath; logic is duplicated verbatim into both HTML templates and must be kept in sync manually.writePluginImportsFile.js— generated runtime loader now routes non-HTTP extension import paths throughgetPublicSubPath; thestartsWith('http')guard misses protocol-relative URLs (//cdn.example.com/ext.js), whichgetPublicSubPaththen corrupts.webpack.pwa.js—output.publicPathis set to''for relative chunk URLs;historyApiFallback.indexloses its leading slash, which may break SPA deep-link fallback during local development.Confidence Score: 4/5
Safe to merge for standard deployments; the protocol-relative URL gap in the generated plugin loader should be fixed before any CDN-hosted extension relies on it.
The core runtime base-path logic is well-structured and the inline-script/webpack-entry timing is correct for the common case. One concrete defect exists in the generated
getRuntimeImportPathhelper: protocol-relative URLs (//host/path) are not excluded fromgetPublicSubPath, so any extension configured with such a URL would load from a mangled path instead of the CDN.platform/app/.webpack/writePluginImportsFile.js — the generated
getRuntimeImportPathguard needs|| path.startsWith('//')to protect protocol-relative CDN URLs.Important Files Changed
getPublicSubPathinto generated plugin imports and rewrites non-HTTP paths at runtime;getRuntimeImportPathmisses protocol-relative URLs (//host/...), corrupting CDN-hosted extension imports.getLocationBasePathFromPathnamehas a knownviewerIndex === 0edge case (deployment at/viewer/) already flagged in prior review threads.__webpack_public_path__from the runtime base path before React renders; correctly imports from@ohif/coreand runs before any dynamic imports in the app lifecycle.<script>now derives and sets<base href>before other head resources load; logic is a verbatim copy ofpublicUrl.tsand must stay in sync manually (flagged in prior threads).publicPathfor relative chunk URLs;historyApiFallback.indexloses its leading slash which may break SPA deep-link fallback in dev-server.../../../core/src/utils/publicUrl(raw TS source) instead of the@ohif/corepackage boundary, as flagged in prior review threads.publicUrl.tsvia both the named-export list and the defaultutilsobject.@ohif/corepackage entry point.import './setRuntimePublicPath'as the first side-effect import, ensuring__webpack_public_path__is set before App renders.Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "Merge master into runtime base path PR" | Re-trigger Greptile