Remove four dead admin services that hardcode http://localhost:8080 - #154
Open
drtechie wants to merge 1 commit into
Open
Remove four dead admin services that hardcode http://localhost:8080#154drtechie wants to merge 1 commit into
drtechie wants to merge 1 commit into
Conversation
LanguageService, RoleService, ScreenService and ServicemasterService each carry
hardcoded base URLs:
http://localhost:8080/Language/iEMR/userLanguage/getData
http://localhost:8080//roleGet
http://localhost:8080//iEMR/UserScreen/getScreenData
http://localhost:8080/iEMR/ServiceMaster/getServiceData
They ignore the environment entirely, so in any deployed build the browser is told
to call localhost — which is the user's own machine, not the server. The doubled
slashes are a further hint that these were never exercised.
Repointing them at ADMIN_API_BASE would not help: none of those five endpoints
exists in Admin-API or Common-API, so the result would be a 404 instead of a
connection failure.
They are also unreachable code. All four classes are injected into ZERO
constructors; the only references are the import and provider registration in
core.module.ts. (Note the RoleService that IS widely used across the app is
services/state-serviceline-role.service — a different class, untouched here.)
So this deletes the four files and their registrations rather than fixing URLs
that point at endpoints which no longer exist. No behaviour change: nothing could
call them.
After this change `grep -rn 'localhost:8080' src/` returns nothing.
Found while standing up AMRIT for a new partner: inspecting the compiled bundle
served from the deployment showed 8 references to localhost, which is what led
back to these four files. Present identically on release-3.4.0, release-3.6.1 and
main, so every deployment ships them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (4)
📝 WalkthroughWalkthroughThe change removes four admin services and their registrations from ChangesAdmin service removal
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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. Comment |
|
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.
Four services in
src/app/core/services/adminServices/carry hardcoded base URLs that ignore the environment entirely:AdminLanguage/language.service.tshttp://localhost:8080/Language/iEMR/userLanguage/getDataAdminRole/role.service.tshttp://localhost:8080//roleGetAdminScreen/screen.service.tshttp://localhost:8080//iEMR/UserScreen/getScreenDataAdminService/servicemaster.service.tshttp://localhost:8080/iEMR/ServiceMaster/getServiceDataIn any deployed build the browser is told to call
localhost— the user's own machine, not the server. The doubled slashes are a further hint these were never exercised.Why the fix is deletion rather than repointing at ADMIN_API_BASE
Two reasons:
1. The endpoints no longer exist. None of
roleGet,roleSave,userLanguage/getData,UserScreen/getScreenDataorServiceMaster/getServiceDatais present in Admin-API or Common-API. Repointing the base URL would turn a connection failure into a 404.2. The code is unreachable. All four classes are injected into zero constructors. Their only references are the import and provider registration in
core.module.ts:Worth noting for reviewers: the
RoleServicethat is used widely across the app isservices/state-serviceline-role.service— a different class, untouched by this PR.So there is no behaviour change. Nothing could call these.
Result
5 files changed, 217 deletions, 1 insertion. Afterwards:
Verification
Built from this branch on PSMRI Jenkins with the real toolchain (NODE_20, Angular 16): SUCCESS. Inspected the compiled bundle actually served by the deployment afterwards —
localhostreferences went from 8 to 0, and the Admin UI still loads and serves normally.How this was found
Standing up AMRIT for a new partner. Auditing the served
main.*.jsfor URLs that should not be in a partner build surfaced 8localhostreferences, which led back to these four files. They are identical onrelease-3.4.0,release-3.6.1andmain, so every deployment currently ships them.🤖 Generated with Claude Code
Summary by CodeRabbit