Feature/dental UI saving and auth#5958
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
✅ 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}
| <div className="border-muted mx-1.5 h-[25px] border-r"></div> | ||
| {UserInfo} |
There was a problem hiding this comment.
The separator <div> is placed unconditionally before {UserInfo}. When Firebase is disabled (the common case), UserInfo is undefined, but the divider still renders — adding a spurious vertical rule in the header for every non-Firebase user.
| <div className="border-muted mx-1.5 h-[25px] border-r"></div> | |
| {UserInfo} | |
| {UserInfo && ( | |
| <> | |
| <div className="border-muted mx-1.5 h-[25px] border-r"></div> | |
| {UserInfo} | |
| </> | |
| )} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: platform/ui-next/src/components/Header/Header.tsx
Line: 124-125
Comment:
**Divider always renders when UserInfo is undefined**
The separator `<div>` is placed unconditionally before `{UserInfo}`. When Firebase is disabled (the common case), `UserInfo` is `undefined`, but the divider still renders — adding a spurious vertical rule in the header for every non-Firebase user.
```suggestion
{UserInfo && (
<>
<div className="border-muted mx-1.5 h-[25px] border-r"></div>
{UserInfo}
</>
)}
```
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 rendered unconditionally in WorkList
<FirebaseUserInfo /> is always passed as UserInfo regardless of whether Firebase is configured. In non-Firebase deployments FirebaseUserInfo returns <></>, but the separator <div> in Header.tsx will still render (see related header comment). The component should only be passed when Firebase is actually enabled — mirroring the guard used in App.tsx.
| UserInfo={<FirebaseUserInfo />} | |
| UserInfo={process.env.REACT_APP_FIREBASE_API_KEY ? <FirebaseUserInfo /> : undefined} |
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` rendered unconditionally in WorkList**
`<FirebaseUserInfo />` is always passed as `UserInfo` regardless of whether Firebase is configured. In non-Firebase deployments `FirebaseUserInfo` returns `<></>`, but the separator `<div>` in `Header.tsx` will still render (see related header comment). The component should only be passed when Firebase is actually enabled — mirroring the guard used in `App.tsx`.
```suggestion
UserInfo={process.env.REACT_APP_FIREBASE_API_KEY ? <FirebaseUserInfo /> : undefined}
```
How can I resolve this? If you propose a fix, please make it concise.| 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 not awaited and the onSignIn interface declares () => void, so any rejection from signInWithPopup is swallowed inside FirebaseAuthRoutes.handleSignIn. The catch block in handleGoogleSignIn never fires on async failures. When the popup is dismissed or a network error occurs, isSigningIn stays true permanently — the button shows "Redirecting…" with no way to retry. onSignIn should be typed as () => Promise<void> and awaited here so errors can be caught and displayed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: platform/app/src/routes/LoginPage.tsx
Line: 14-22
Comment:
**Sign-in errors are never surfaced to the user**
`onSignIn()` is not awaited and the `onSignIn` interface declares `() => void`, so any rejection from `signInWithPopup` is swallowed inside `FirebaseAuthRoutes.handleSignIn`. The `catch` block in `handleGoogleSignIn` never fires on async failures. When the popup is dismissed or a network error occurs, `isSigningIn` stays `true` permanently — the button shows "Redirecting…" with no way to retry. `onSignIn` should be typed as `() => Promise<void>` and awaited here so errors can be caught and displayed.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| REACT_APP_FIREBASE_API_KEY= | ||
| REACT_APP_FIREBASE_AUTH_DOMAIN= | ||
| REACT_APP_FIREBASE_PROJECT_ID= | ||
| REACT_APP_FIREBASE_STORAGE_BUCKET= | ||
| REACT_APP_FIREBASE_MESSAGING_SENDER_ID= |
There was a problem hiding this comment.
.env is tracked in git — .gitignore change is ineffective
platform/app/.env is already a tracked file, so adding it to .gitignore in this PR will not stop git from tracking future changes to it. Any developer who fills in real Firebase API keys will commit them to history. The conventional approach is:
- Rename
platform/app/.env→platform/app/.env.example(keep tracked as a template with empty values). - Have each developer create their own
platform/app/.envlocally (untracked). - Run
git rm --cached platform/app/.envto stop tracking the current file.
The .gitignore entry for .env is correct as written, but it only takes effect once the file is untracked.
Prompt To Fix With AI
This is a comment left during a code review.
Path: platform/app/.env
Line: 12-17
Comment:
**`.env` is tracked in git — `.gitignore` change is ineffective**
`platform/app/.env` is already a tracked file, so adding it to `.gitignore` in this PR will not stop git from tracking future changes to it. Any developer who fills in real Firebase API keys will commit them to history. The conventional approach is:
1. Rename `platform/app/.env` → `platform/app/.env.example` (keep tracked as a template with empty values).
2. Have each developer create their own `platform/app/.env` locally (untracked).
3. Run `git rm --cached platform/app/.env` to stop tracking the current file.
The `.gitignore` entry for `.env` is correct as written, but it only takes effect once the file is untracked.
How can I resolve this? If you propose a fix, please make it concise.| userAuthenticationService.set({ enabled: true }); | ||
| userAuthenticationService.setServiceImplementation({ | ||
| getAuthorizationHeader, | ||
| handleUnauthenticated: () => null, | ||
| reset: () => signOut(firebaseAuth), | ||
| }); | ||
| }, []); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
useEffect
userAuthenticationService is referenced inside the first useEffect but is not listed in its dependency array. ESLint's exhaustive-deps rule would flag this. While the component is likely mounted once, adding the dependency makes the intent explicit and prevents subtle bugs if the service reference ever changes.
| userAuthenticationService.set({ enabled: true }); | |
| userAuthenticationService.setServiceImplementation({ | |
| getAuthorizationHeader, | |
| handleUnauthenticated: () => null, | |
| reset: () => signOut(firebaseAuth), | |
| }); | |
| }, []); | |
| useEffect(() => { | |
| useEffect(() => { | |
| userAuthenticationService.set({ enabled: true }); | |
| userAuthenticationService.setServiceImplementation({ | |
| getAuthorizationHeader, | |
| handleUnauthenticated: () => null, | |
| reset: () => signOut(firebaseAuth), | |
| }); | |
| }, [userAuthenticationService]); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: platform/app/src/utils/FirebaseAuthRoutes.tsx
Line: 41-49
Comment:
**Missing dependency in `useEffect`**
`userAuthenticationService` is referenced inside the first `useEffect` but is not listed in its dependency array. ESLint's `exhaustive-deps` rule would flag this. While the component is likely mounted once, adding the dependency makes the intent explicit and prevents subtle bugs if the service reference ever changes.
```suggestion
useEffect(() => {
userAuthenticationService.set({ enabled: true });
userAuthenticationService.setServiceImplementation({
getAuthorizationHeader,
handleUnauthenticated: () => null,
reset: () => signOut(firebaseAuth),
});
}, [userAuthenticationService]);
```
How can I resolve this? If you propose a fix, please make it concise.1 participant
C) User Preference Saving & Authentication
📝 Create & clone a branch to work on Task C
📝 Setup Firebase Project and Create a Login Page
📝 Create Logout button
📝 Save User Preference on Theme
Greptile Summary
This PR adds Google/Firebase authentication (login page, auth guard, user-info dropdown, logout) and a dental-theme preference saved to
localStorage. The integration is feature-flagged behindREACT_APP_FIREBASE_API_KEYand is mutually exclusive with existing OIDC, keeping the default path unaffected.Three issues need attention before merge:
<div>separator inHeader.tsxis unconditional, adding a phantom divider in every non-Firebase build.WorkListalways passes<FirebaseUserInfo />asUserInforegardless of config, amplifying the divider problem.onSignIn()inLoginPageis not awaited, so popup-dismissed / network errors are silently swallowed and the button stays permanently disabled.Confidence Score: 3/5
Not safe to merge — the unconditional header divider is a visible regression for all non-dental users, and the login error-handling bug leaves users stuck with a disabled button.
Three P1 issues: a UI regression in the shared Header component (affects all users), an always-on Firebase import in WorkList (compounded divider issue), and a broken error-propagation path in the login flow that produces a stuck UI state. The
.envtracking issue is a latent security risk. All are straightforward fixes but the header regression alone touches the critical shared path.platform/ui-next/src/components/Header/Header.tsx,platform/app/src/routes/WorkList/WorkList.tsx,platform/app/src/routes/LoginPage.tsx,platform/app/.envSecurity Review
platform/app/.env): Firebase API keys are added to a git-tracked.envfile. Adding.envto.gitignoredoes not untrack an already-tracked file, so if a developer fills in real credentials, they will be committed to history. Use.env.exampleas the template and untrack.envwithgit rm --cached.Important Files Changed
UserInfoslot to header but the separator<div>renders unconditionally, producing a spurious divider for all non-Firebase builds.<FirebaseUserInfo />asUserInforegardless of whether Firebase is configured, causing spurious dividers in non-Firebase deployments.onSignIn()is not awaited so async errors are never caught, leaving the button permanently disabled on failure with no error shown..envfile;.gitignorechange is ineffective for already-tracked files, risking future credential exposure.onAuthStateChangedfor state management; minor dependency-array gap inuseEffect.referrerPolicy="no-referrer"on avatar image appropriately.REACT_APP_FIREBASE_API_KEY) that is mutually exclusive with OIDC; logic is clean and non-breaking for existing auth flows.restore()is called at module-evaluation time as a side effect, which is acceptable for browser builds.Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "📝 Save User Preference on Theme" | Re-trigger Greptile
(2/5) Greptile learns from your feedback when you react with thumbs up/down!