Feature/dental UI refactor and deploy#5959
Conversation
📝 Create a new mode [dental-ui] "Dental Mode" base on Longitudinal Mode and to refrain from editing original code
…o refrain from editing original code 📝 Add a button to toggle theme on the Header (change UI's color, font, icon)
○ Top-left: Current image ○ Top-right: Prior exam (Empty if none) ○ Bottom: Bitewing placeholders)
Feature/dental UI mode
📝 Create new set of Measurement tool for Dental Mode which save its measurement on the new Dental Measurement Panel that automatically renames depending on tool name with auto increment. ○ PA Length : uses Periapical Length Tool ○ Canal Angle : uses Canal Angle Tool ○ Crown Width : uses Periapical Length Tool ○ Root Length : uses Periapical Length Tool
…ements in JSON format
Feature/dental UI measurements
📝 Setup Firebase Project and Create a Login Page
Feature/dental UI saving and auth
📝 Create, Build and Deploy the Project using Docker to Firebase 📝 Full Scale Testing 📝 Fix Possible Bugs
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| setError(null); | ||
| try { | ||
| onSignIn(); | ||
| } catch (err: any) { | ||
| console.error('Google sign-in error:', err); | ||
| setError(err.message || 'Sign-in failed. Please try again.'); | ||
| setIsSigningIn(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
onSignIn() is called without await, so errors from the async handleSignIn in FirebaseAuthRoutes (popup closed, network error, etc.) are silently swallowed and never reach the catch block here. When sign-in fails, isSigningIn remains true indefinitely — the button stays disabled showing "Redirecting…" and setError is never called. The user must refresh the page to retry.
| setError(null); | |
| try { | |
| onSignIn(); | |
| } catch (err: any) { | |
| console.error('Google sign-in error:', err); | |
| setError(err.message || 'Sign-in failed. Please try again.'); | |
| setIsSigningIn(false); | |
| } | |
| }; | |
| const handleGoogleSignIn = async () => { | |
| setIsSigningIn(true); | |
| setError(null); | |
| try { | |
| await onSignIn(); | |
| } catch (err: any) { | |
| console.error('Google sign-in error:', err); | |
| setError(err.message || 'Sign-in failed. Please try again.'); | |
| setIsSigningIn(false); | |
| } | |
| }; |
handleSignIn in FirebaseAuthRoutes should also re-throw errors instead of silently logging them, so callers can handle them.
Prompt To Fix With AI
This is a comment left during a code review.
Path: platform/app/src/routes/LoginPage.tsx
Line: 13-21
Comment:
**Sign-in button permanently stuck after a failed attempt**
`onSignIn()` is called without `await`, so errors from the async `handleSignIn` in `FirebaseAuthRoutes` (popup closed, network error, etc.) are silently swallowed and never reach the `catch` block here. When sign-in fails, `isSigningIn` remains `true` indefinitely — the button stays disabled showing "Redirecting…" and `setError` is never called. The user must refresh the page to retry.
```suggestion
const handleGoogleSignIn = async () => {
setIsSigningIn(true);
setError(null);
try {
await onSignIn();
} catch (err: any) {
console.error('Google sign-in error:', err);
setError(err.message || 'Sign-in failed. Please try again.');
setIsSigningIn(false);
}
};
```
`handleSignIn` in `FirebaseAuthRoutes` should also re-throw errors instead of silently logging them, so callers can handle them.
How can I resolve this? If you propose a fix, please make it concise.| isReturnEnabled={false} | ||
| WhiteLabeling={appConfig.whiteLabeling} | ||
| showPatientInfo={PatientInfoVisibility.DISABLED} | ||
| UserInfo={<FirebaseUserInfo />} |
There was a problem hiding this comment.
FirebaseUserInfo hardcoded, bypassing the customization system
<FirebaseUserInfo /> is always passed here regardless of whether Firebase is enabled, unlike ViewerHeader.tsx which reads ohif.userInfo from the customizationService. This means:
- The Firebase SDK is always bundled and imported even in non-Firebase OHIF deployments.
- Any other
ohif.userInfocustomization registered by a downstream consumer is silently ignored in the WorkList header.
The WorkList should mirror the ViewerHeader pattern — either read customizationService.getCustomization('ohif.userInfo') or gate on the same firebaseEnabled flag used in App.tsx.
Prompt To Fix With AI
This is a comment left during a code review.
Path: platform/app/src/routes/WorkList/WorkList.tsx
Line: 561
Comment:
**`FirebaseUserInfo` hardcoded, bypassing the customization system**
`<FirebaseUserInfo />` is always passed here regardless of whether Firebase is enabled, unlike `ViewerHeader.tsx` which reads `ohif.userInfo` from the `customizationService`. This means:
1. The Firebase SDK is always bundled and imported even in non-Firebase OHIF deployments.
2. Any other `ohif.userInfo` customization registered by a downstream consumer is silently ignored in the WorkList header.
The WorkList should mirror the `ViewerHeader` pattern — either read `customizationService.getCustomization('ohif.userInfo')` or gate on the same `firebaseEnabled` flag used in `App.tsx`.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| # Firebase env vars — passed as --build-arg at docker build time | ||
| ARG REACT_APP_FIREBASE_API_KEY | ||
| ARG REACT_APP_FIREBASE_AUTH_DOMAIN | ||
| ARG REACT_APP_FIREBASE_PROJECT_ID | ||
| ARG REACT_APP_FIREBASE_STORAGE_BUCKET | ||
| ARG REACT_APP_FIREBASE_MESSAGING_SENDER_ID | ||
| ARG REACT_APP_FIREBASE_APP_ID | ||
| ENV REACT_APP_FIREBASE_API_KEY=${REACT_APP_FIREBASE_API_KEY} | ||
| ENV REACT_APP_FIREBASE_AUTH_DOMAIN=${REACT_APP_FIREBASE_AUTH_DOMAIN} | ||
| ENV REACT_APP_FIREBASE_PROJECT_ID=${REACT_APP_FIREBASE_PROJECT_ID} | ||
| ENV REACT_APP_FIREBASE_STORAGE_BUCKET=${REACT_APP_FIREBASE_STORAGE_BUCKET} |
There was a problem hiding this comment.
Values passed via --build-arg are permanently stored in each build layer and visible to anyone with access to the image via docker history <image>. While Firebase web API keys are not highly sensitive secrets (they're embedded in client JS), this pattern sets a precedent that could leak more sensitive values if the approach is reused. Consider writing a .env-style file at build time instead of using build args for credentials, or accept the trade-off explicitly in team policy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Dockerfile
Line: 74-85
Comment:
**Firebase credentials recorded in Docker image build history**
Values passed via `--build-arg` are permanently stored in each build layer and visible to anyone with access to the image via `docker history <image>`. While Firebase web API keys are not highly sensitive secrets (they're embedded in client JS), this pattern sets a precedent that could leak more sensitive values if the approach is reused. Consider writing a `.env`-style file at build time instead of using build args for credentials, or accept the trade-off explicitly in team policy.
How can I resolve this? If you propose a fix, please make it concise.| USE_HASH_ROUTER=false | ||
|
|
||
| REACT_APP_FIREBASE_API_KEY= | ||
| REACT_APP_FIREBASE_AUTH_DOMAIN= | ||
| REACT_APP_FIREBASE_PROJECT_ID= | ||
| REACT_APP_FIREBASE_STORAGE_BUCKET= |
There was a problem hiding this comment.
.env file with Firebase keys committed to the repository
Although the values are empty placeholders here, the .env file itself is now tracked in git. The .gitignore addition in this PR (# Environment variables / .env) does not untrack a file that was already committed — git rm --cached platform/app/.env would be needed. A developer filling in real credentials locally won't be warned that the file is tracked and could accidentally commit them. Consider removing the file from tracking and replacing it with a .env.example or .env.template file containing only keys (no values).
Prompt To Fix With AI
This is a comment left during a code review.
Path: platform/app/.env
Line: 11-16
Comment:
**`.env` file with Firebase keys committed to the repository**
Although the values are empty placeholders here, the `.env` file itself is now tracked in git. The `.gitignore` addition in this PR (`# Environment variables / .env`) does not untrack a file that was already committed — `git rm --cached platform/app/.env` would be needed. A developer filling in real credentials locally won't be warned that the file is tracked and could accidentally commit them. Consider removing the file from tracking and replacing it with a `.env.example` or `.env.template` file containing only keys (no values).
How can I resolve this? If you propose a fix, please make it concise.1 participant
D.) Refactor, Deployment & Testing (Claude, Docker + Firebase)
📝 Create & clone a branch to work on Task D
📝 Create, Build and Deploy the Project using Docker to Firebase
📝 Full Scale Testing
📝 Fix Possible Bugs
📝 Update ReadMe File
Greptile Summary
This PR introduces a new
24x7-dental-uiOHIF extension and mode with dental-specific tools (PA Length, Canal Angle, Crown Width, Root Length), a 2×2 hanging protocol, a tooth selector, and Firebase Google Auth for deployment to Cloud Run + Firebase Hosting.LoginPage.tsx:onSignIn()is not awaited andhandleSignInswallows all errors, so a failed sign-in (popup closed, network error) leaves the button permanently disabled showing "Redirecting…" with no way to retry without a page refresh.WorkList.tsx:<FirebaseUserInfo />is hardcoded unconditionally, bypassing theohif.userInfocustomization service used byViewerHeader. This always bundles the Firebase SDK and silently ignores any otherohif.userInfocustomization registered by downstream consumers.Confidence Score: 4/5
Two P1 defects need fixes before merging: the permanently-stuck login button and the unconditional FirebaseUserInfo in WorkList.
The dental extension and mode code is well-structured and clean. However, the Firebase auth integration has two present defects that affect real users: a broken retry path on the login page and a customization bypass in WorkList. Both are straightforward to fix.
platform/app/src/routes/LoginPage.tsxandplatform/app/src/routes/WorkList/WorkList.tsxneed attention before merge.Security Review
platform/app/.envtracked in git: Empty placeholder values are committed; becausegit rm --cachedwas not run, a developer who fills in real Firebase credentials locally could accidentally commit them in a future change.Dockerfile+deploy.sh): Firebase credentials are passed via--build-arg, which records them in image layer history and are visible viadocker history. Firebase web API keys are not high-sensitivity secrets, but the pattern should be noted.Important Files Changed
onSignIn()not awaited causes the button to stay permanently disabled after a failed sign-in attempt<FirebaseUserInfo />unconditionally, bypassing theohif.userInfocustomization mechanism used in ViewerHeader and always bundling the Firebase SDKhandleSignInare swallowed (contributing to LoginPage P1)firebaseEnabledflag.env, builds Docker image with Firebase build args, deploys to Cloud Run and Firebase HostingPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "📝 Update ReadMe File" | Re-trigger Greptile
(3/5) Reply to the agent's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!