feat: implement admin authentication system#70
Open
lamurian wants to merge 12 commits into
Open
Conversation
Remove duplicate rate limiting tests from auth.test.ts, standardize imports to use @/ alias, and extract idle timer into reusable hook
…smart back navigation Add two recovery options: direct plaintext key validation and email-based recovery that reads admin email from database (prevents tampering). Replace Cancel with Back button in login modal, navigating to last visited public page (home/about/test) via new navigation tracking state. - Add validateRecoveryKey IPC handler for direct key verification - Modify requestRecovery to read email from DB automatically - Implement tabbed RecoveryModal (direct + email paths) - Track lastVisitedPublicPage in navigation store - Add onBack prop to AdminLoginModal with smart navigation - Update translations for new UI elements (EN/ID) - Adjust auth tests to updated requestRecovery signature
Contributor
📝 WalkthroughWalkthroughThis PR implements a complete admin authentication system with bcrypt password hashing, LMK-backed AES-256-GCM encryption for sensitive data, device-bound recovery keys, rate-limited login lockout (5 failures → 1-minute lockout), in-memory sessions with 10-minute TTL, and admin-gated IPC handlers protecting Settings and Data Management pages. ChangesAdmin Authentication System
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
 |
|
Overall Grade |
Security Reliability  Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| JavaScript |  |
May 8, 2026 11:42p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 8 medium 1 high |
| ErrorProne | 22 high |
| Security | 7 high |
| Complexity | 4 medium |
| Comprehensibility | 2 minor |
🟢 Metrics 553 complexity · 46 duplication
Metric Results Complexity 553 Duplication 46
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
coderabbitai
Bot
changed the title
TypeScript flagged that callArgs[2] may be undefined when accessing headers.Authorization. Added explicit runtime guards to verify config object exists and contains Authorization header before access
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/generated-config.ts (1)
1-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
generated-config.tsshould not be in source control.Line 1–20 introduces a committed
src/main/generated-config.ts, but this file is designated as CI-generated and non-versioned. Remove it from git and generate it only in build artifacts.As per coding guidelines: "
src/main/generated-config.ts: ... must never be committed to version control".🤖 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 `@src/main/generated-config.ts` around lines 1 - 20, Remove the committed generated config and stop tracking the auto-generated constants: delete the committed file that declares CONFIG with RECOVERY_WEBHOOK_URL and RECOVERY_WEBHOOK_SECRET from the repo, remove it from git tracking (git rm --cached), add its filename pattern to .gitignore so it is never committed again, and ensure the CI/build pipeline writes the file at build time so the runtime validation block that checks CONFIG.RECOVERY_WEBHOOK_URL and CONFIG.RECOVERY_WEBHOOK_SECRET remains valid.
🟡 Minor comments (7)
src/renderer/types/electronAPI.ts-85-85 (1)
85-85:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
authRequestRecovery()signature contradicts AUTH.md §2.2.The renderer API no longer accepts an email argument (per PR objectives, the email is read from the DB), but
AUTH.md§2.2 step 3 still walks the user through "Enter your registered email address" and describes a case-insensitive comparison against user input. Either the docs need to be updated to reflect the no-input flow, or the API needs to accept an email so the verification described in the docs actually happens. The current shape also means anyone with local access can trigger an outbound recovery email without proving they know the registered address — please confirm this is the intended threat model.🤖 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 `@src/renderer/types/electronAPI.ts` at line 85, authRequestRecovery currently takes no email which contradicts AUTH.md §2.2 and allows triggering recovery without proving knowledge of the registered address; change the renderer API signature authRequestRecovery to accept an email parameter (authRequestRecovery: (email: string) => Promise<{ success: boolean; message: string }>) and in the implementation (the function that sends recovery emails) look up the stored email from the DB and perform a case-insensitive comparison (e.g., normalize both to lower-case) before sending any outbound recovery email, returning a suitable success/failure message; alternatively, if you intend the no-input flow, update AUTH.md to remove the "enter your registered email" step and document the new threat model.src/main/key-management.ts-115-133 (1)
115-133:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
getOrCreateDeviceUUIDhas a SELECT-then-INSERT race.Two concurrent callers (or a re-entrant flow during init) will both read no row and both INSERT. If
test_config.keycarries a UNIQUE/PK constraint, the second insert throws; if not, you end up with twodevice_uuidrows andSELECTbecomes nondeterministic. Either wrap in a transaction or use an upsert and read back.♻️ Proposed atomic upsert
- const uuid = randomUUID(); - db.prepare('INSERT INTO test_config (key, value) VALUES (?, ?)').run('device_uuid', uuid); - - return uuid; + const uuid = randomUUID(); + db.prepare( + 'INSERT INTO test_config (key, value) VALUES (?, ?) ON CONFLICT(key) DO NOTHING' + ).run('device_uuid', uuid); + const stored = db + .prepare('SELECT value FROM test_config WHERE key = ?') + .get('device_uuid') as { value: string }; + return stored.value;🤖 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 `@src/main/key-management.ts` around lines 115 - 133, getOrCreateDeviceUUID currently does a SELECT then INSERT, which causes a race when two callers run concurrently; change it to perform an atomic upsert/read inside a transaction or a single upsert statement and then SELECT the value to guarantee determinism: wrap the logic around db.prepare(...) calls (the SELECT 'SELECT value FROM test_config WHERE key = ?' and the INSERT 'INSERT INTO test_config (key, value) VALUES (?, ?)') in a transaction or replace them with a single UPSERT (INSERT ... ON CONFLICT DO UPDATE/DO NOTHING) and then fetch the row to return the stored UUID; ensure the function still returns the existing UUID when present and handles errors the same way.AUTH.md-133-133 (1)
133-133:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRecovery key length contradicts §3.3.
This says "32-character hexadecimal recovery key", but §3.3 (and the renderer hint
64 karakter heksadesimal) describes a 32‑byte / 64‑character hex string. Users will be confused, and the validation regex will reject a 32‑char input.📝 Proposed fix
-After successful registration, a **32-character hexadecimal recovery key** is displayed once. This key is your only way to reset your password if you forget it. +After successful registration, a **64-character hexadecimal recovery key** (32 random bytes) is displayed once. This key is your only way to reset your password if you forget it.🤖 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 `@AUTH.md` at line 133, Update the contradictory description in AUTH.md so the recovery key length matches §3.3 and the validation regex: change the phrase "32-character hexadecimal recovery key" to "64-character hexadecimal (32‑byte) recovery key" or "32‑byte (64‑character) hexadecimal recovery key" and ensure any nearby renderer hint (`64 karakter heksadesimal`) is consistent with this wording so users and validation align.src/main/auth.test.ts-279-290 (1)
279-290:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest does not actually verify expiry is extended.
Two consecutive logins overwrite expiry; the assertion
expiry > Date.now()passes for any fresh session. To verify extension, capture the expiry after login, advance fake timers (e.g., 5 min), callverifySession, then assert the new expiry is strictly greater than the captured one.♻️ Suggested rewrite
- it('should extend session expiry on valid verifySession call', async () => { - const { loginAdmin, verifySession } = auth; - - await loginAdmin('correctPassword', 1); - const token = (await loginAdmin('correctPassword', 1)).sessionToken; - - const valid = verifySession(token, 1); - expect(valid).toBe(true); - - const expiry = parseInt(db._seeds['session_expiry'], 10); - expect(expiry).toBeGreaterThan(Date.now()); - }); + it('should extend session expiry on valid verifySession call', async () => { + const { loginAdmin, verifySession } = auth; + vi.useFakeTimers(); + const { sessionToken } = await loginAdmin('correctPassword', 1); + const initialExpiry = parseInt(db._seeds['session_expiry'], 10); + + vi.advanceTimersByTime(5 * 60 * 1000); + expect(verifySession(sessionToken, 1)).toBe(true); + + const extendedExpiry = parseInt(db._seeds['session_expiry'], 10); + expect(extendedExpiry).toBeGreaterThan(initialExpiry); + vi.useRealTimers(); + });🤖 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 `@src/main/auth.test.ts` around lines 279 - 290, The test currently only checks a fresh session expiry; change it to capture the initial expiry after login (use loginAdmin to get sessionToken and read db._seeds['session_expiry']), advance fake timers by a period (e.g., 5 minutes), call verifySession(token, 1) to trigger extension, then read the updated db._seeds['session_expiry'] and assert the new expiry is strictly greater than the captured initial expiry; ensure you use the same token and user id (verifySession and loginAdmin) and restore timers if you mock them.src/renderer/store/useAuthStore.test.ts-39-47 (1)
39-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
resetStore()omitshasLoaded, risking state leakage between tests.The store defines
hasLoaded: booleanin its interface and initializes it tofalse, butresetStore()doesn't reset it. If one test sets it totrue, the next test will inherit that value. IncludehasLoaded: falsein the reset, or spread the store's initial state.🤖 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 `@src/renderer/store/useAuthStore.test.ts` around lines 39 - 47, The test helper resetStore currently calls useAuthStore.setState but omits the hasLoaded property, which can cause state leakage between tests; update resetStore to include hasLoaded: false in the object passed to useAuthStore.setState (or alternatively spread the store's initial state) so that hasLoaded is always reset; locate the resetStore function in useAuthStore.test.ts and modify the setState payload to explicitly set hasLoaded: false (or use the store's initialState spread) to ensure clean test isolation.src/renderer/pages/Settings.tsx-28-123 (1)
28-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize hardcoded English strings.
'Password is required'(line 28),placeholder="Enter current password"(line 85), and'Removing...'(line 123) bypassuseTranslationand will appear in English foridusers. Add corresponding keys tosrc/locales/{en,id}/...and uset(...).🌐 Suggested fix
- setError('Password is required'); + setError(t('settings.removeAdminPasswordRequired')); @@ - placeholder="Enter current password" + placeholder={t('settings.removeAdminPasswordPlaceholder')} @@ - {isLoading ? 'Removing...' : t('settings.removeAdmin')} + {isLoading ? t('settings.removingAdmin') : t('settings.removeAdmin')}🤖 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 `@src/renderer/pages/Settings.tsx` around lines 28 - 123, Replace hardcoded UI strings with translation keys: change the literal 'Password is required' used in handleConfirm (setError call), the input placeholder "Enter current password" on the remove-admin-password input, and the loading label 'Removing...' in the confirm button to use t('...') calls; add matching translation keys (for example settings.passwordRequired, settings.removeAdminPlaceholder, settings.removing) to both English and Indonesian locale files so the new t(...) lookups return localized text.src/main/auth.ts-444-448 (1)
444-448:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFalsy guard silently returns an unusable session token when
webContentsIdisundefined.
if (webContentsId)is false for bothundefinedand0. WhenwebContentsIdis absent, no session is stored inactiveSessions, yet a freshsessionTokenis returned to the caller — a token that will fail every subsequentverifySessionorrequireAdmincheck without any indication of why.🔧 Proposed fix
- if (webContentsId) { - activeSessions.set(sessionToken, { webContentsId, expiry }); - } - - return { sessionToken }; + if (webContentsId === undefined) { + throw new Error('webContentsId is required to create a session'); + } + activeSessions.set(sessionToken, { webContentsId, expiry }); + return { sessionToken };🤖 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 `@src/main/auth.ts` around lines 444 - 448, The current falsy guard `if (webContentsId)` treats 0 as absent and silently returns a sessionToken when `webContentsId` is actually undefined, producing unusable tokens for `verifySession`/`requireAdmin`; change the guard to an explicit null/undefined check (e.g., `if (webContentsId !== undefined && webContentsId !== null)`) so numeric 0 is accepted, and when `webContentsId` is missing explicitly fail instead of returning a token (throw or return an error) so callers receive a clear failure rather than a silent unusable `sessionToken`; reference `webContentsId`, `activeSessions`, and `sessionToken` to locate and update the logic.
🧹 Nitpick comments (4)
src/locales/id/translation.json (1)
280-280: 💤 Low valuePrefer
AutentikasioverOtentikasi.KBBI/PUEBI's preferred form is
Autentikasi.Otentikasiis widely understood but considered a non-standard variant. Worth aligning with whichever spelling the rest of the app uses for consistency.🤖 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 `@src/locales/id/translation.json` at line 280, Update the translation value for the JSON key where "title": "Otentikasi Admin" to use the preferred spelling "Autentikasi Admin"; locate the entry in src/locales/id/translation.json with the "title" key currently set to "Otentikasi Admin" and replace "Otentikasi" with "Autentikasi" to match KBBI/PUEBI and the app's consistent spelling.src/renderer/store/useNavigation.ts (1)
23-29: 💤 Low valueTighten the
publicPagestyping.
PublicPageincludesnull, so typing the lookup array asPublicPage[]is misleading and forces theas PublicPagecast. A non-null tuple makes the intent crisper and removes the cast.♻️ Proposed tweak
setPage: (page) => { set({ currentPage: page }); - const publicPages: PublicPage[] = ['home', 'about', 'test']; - if (publicPages.includes(page as PublicPage)) { - set({ lastVisitedPublicPage: page as PublicPage }); + const publicPages = ['home', 'about', 'test'] as const; + if ((publicPages as readonly string[]).includes(page)) { + set({ lastVisitedPublicPage: page as PublicPage }); } },🤖 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 `@src/renderer/store/useNavigation.ts` around lines 23 - 29, The publicPages lookup is typed as PublicPage[] which includes null and forces an unnecessary cast in setPage; change the lookup to a non-null literal tuple (e.g. const publicPages = ['home','about','test'] as const) or a readonly array of the concrete page string literals, then use that in setPage (remove the `as PublicPage` cast) and keep assigning lastVisitedPublicPage when publicPages.includes(page as unknown as typeof publicPages[number]) is true — better: narrow page first to exclude null (e.g. if (page && publicPages.includes(page as any))) or use a typed guard so setPage, publicPages, PublicPage and lastVisitedPublicPage align without casts.src/main/key-management.ts (1)
52-74: ⚡ Quick winCache the LMK to avoid hitting the OS keychain on every encrypt/decrypt.
Both
encryptWithLMKanddecryptWithLMKcallgetOrCreateLMK()on each invocation, which means every email/recovery encrypt or decrypt round-trips throughkeytar. On macOS Keychain / Windows DPAPI this is a non-trivial syscall (and on Linux libsecret can prompt). For login/recovery hot paths this adds up.A simple module-level memoization is sufficient since the LMK is immutable for the process lifetime:
♻️ Proposed cache
+let cachedLMK: string | null = null; + export async function getOrCreateLMK(): Promise<string> { + if (cachedLMK) return cachedLMK; try { const existingKey = await keytar.getPassword(KEYTAR_SERVICE, KEYTAR_ACCOUNT); if (existingKey) { + cachedLMK = existingKey; return existingKey; } // Generate 32 random bytes for AES-256 const newKey = randomBytes(32).toString('hex'); await keytar.setPassword(KEYTAR_SERVICE, KEYTAR_ACCOUNT, newKey); + cachedLMK = newKey; return newKey; } catch (error) { console.error('[KeyManagement] Error accessing system keychain:', error); throw new Error('Failed to retrieve or create Local Master Key'); } }Also applies to: 84-106
🤖 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 `@src/main/key-management.ts` around lines 52 - 74, encryptWithLMK and decryptWithLMK call getOrCreateLMK() on every invocation causing expensive keychain calls; add a module-level cache (e.g., lmkHexCache or lmkPromiseCache) to store the LMK hex (or the ongoing promise) so subsequent calls reuse it instead of calling keytar each time. Update both encryptWithLMK and decryptWithLMK to check the cache first, set the cache when getOrCreateLMK() resolves, and consider caching the promise to avoid duplicate concurrent getOrCreateLMK() calls; keep the cache immutable for process lifetime and use the cached value (or awaited promise) when deriving the key buffer in createCipheriv/createDecipheriv.src/main/auth.ts (1)
107-108: ⚡ Quick win
Date.now()in main process violates the timing guideline.The coding guidelines require
process.hrtime.bigint()for timing in the main process. The in-memory session expiry calculations (lines 128, 169, 175, 267, 442, 480) are a direct use case for the monotonic high-resolution timer. The DB-persisted epoch timestamps (lockout_until, JWTiat/exp) are a legitimate exception since hrtime is relative to process start and cannot represent absolute calendar time across restarts — those may remain asDate.now().♻️ Proposed refactor for in-memory session expiry
-const SESSION_DURATION = 10 * 60 * 1000; // 10 minutes +const SESSION_DURATION = BigInt(10 * 60) * BigInt(1_000_000_000); // 10 minutes in ns interface Session { webContentsId: number; - expiry: number; + expiry: bigint; } // loginAdmin — create session -const expiry = Date.now() + SESSION_DURATION; +const expiry = process.hrtime.bigint() + SESSION_DURATION; // verifySession — check & extend -if (Date.now() > session.expiry) { +if (process.hrtime.bigint() > session.expiry) { -const newExpiry = Date.now() + SESSION_DURATION; +const newExpiry = process.hrtime.bigint() + SESSION_DURATION;As per coding guidelines: "Use
process.hrtime.bigint()for timing in the main process".Also applies to: 128-128, 150-150, 169-169, 175-175
🤖 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 `@src/main/auth.ts` around lines 107 - 108, The code uses Date.now() for in-process timing (e.g., computing remaining = Math.ceil((lockoutUntil - Date.now()) / 1000)) which violates the guideline; change in-memory expiry/lockout logic to use process.hrtime.bigint() (store in-memory expiry times as BigInt nanoseconds) and compute remaining seconds via bigint arithmetic (e.g., (lockoutUntilNs - process.hrtime.bigint()) / 1_000_000_000n, then convert to Number and Math.ceil if needed). Update all in-memory checks referencing lockoutUntil and other in-process expiries in auth.ts (the same pattern at the other identified sites) to use the BigInt hrtime values and only keep Date.now()/epoch seconds for DB-persisted fields (lockout_until, JWT iat/exp).
🤖 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 @.github/actions/electron-build-common/action.yml:
- Around line 38-73: Remove the CI steps that embed the recovery secret into
build artifacts: delete the "Generate main config from secrets" and "Copy config
to dist" actions that write RECOVERY_WEBHOOK_SECRET into
src/main/generated-config.ts and dist/main/generated-config.ts, and delete the
"Create .env for renderer" action that writes VITE_RECOVERY_WEBHOOK_SECRET into
the renderer .env; keep only non-secret values (e.g. VITE_RECOVERY_WEBHOOK_URL
if needed) and ensure RECOVERY_WEBHOOK_SECRET is never written into
generated-config.ts or any VITE_* env var so the secret remains available only
to the main process at runtime.
In @.gitignore:
- Around line 41-43: Restore broad env-file ignore rules in .gitignore: re-add a
pattern to ignore all variants like .env.* (covering .env.production,
.env.staging, etc.) while keeping .env.example tracked via a negation rule
(!.env.example). Ensure the file still ignores .env, .env.local and .env.*.local
as present in the diff and add the general .env.* pattern plus the explicit
exception for .env.example so secrets cannot be committed but the example file
remains in source control.
In `@AUTH.md`:
- Around line 204-213: Update Section "2.4 If the Recovery Key Is Lost" to
replace the misleading "Uninstall and reinstall" instruction with a clear
two-step reset procedure: instruct users to (1) delete the application's
userData SQLite DB (the test_config table contains admin auth and test results
per Appendix A) if they want to remove auth and data, and (2) delete the OS
keychain entry created by keytar (named "focus-auth" / "local-master-key") from
macOS Keychain / Windows Credential Manager so the LMK can be regenerated on
next launch; explicitly state the data-loss tradeoff (deleting the DB removes
test results, deleting only the keychain entry will not reset auth because the
DB still holds auth state), and update the warning text to reflect these exact
steps and consequences.
In `@src/main/auth.ts`:
- Line 63: Replace synchronous bcrypt.hashSync calls with the async bcrypt.hash
variant to avoid blocking the main process: change each occurrence (e.g., the
passwordHash assignment in src/main/auth.ts where passwordHash is set via
bcrypt.hashSync(password, 12)) to await bcrypt.hash(password, 12) (ensure the
enclosing async function remains async), keep the same variable name, and apply
the same one-line edit to the other five call sites (the other bcrypt.hashSync
usages referenced in the review). Optionally factor the salt rounds into a
SALT_ROUNDS constant and handle any promise errors with try/catch where
appropriate.
- Around line 214-260: After deleteAdmin (and similarly in changePassword),
actively revoke existing sessions so old tokens stop working: locate the
activeSessions in-memory store and remove/clear all entries (e.g.,
activeSessions.clear() or iterate and delete each key) and also remove any
persistent test_sessions rows (the code already deletes test_sessions when
wipeData is true — ensure it always deletes or update session records to
revoked). Update any session-related DB fields (e.g., session_expiry or a
server-side auth_version) so requireAdmin/isAuthenticated checks fail for
previously issued tokens; implement this logic immediately after the password
verification and before returning from deleteAdmin and changePassword.
In `@src/main/config.ts`:
- Around line 6-7: The current config exports RECOVERY_WEBHOOK_SECRET (and
RECOVERY_WEBHOOK_URL) with unsafe hardcoded fallbacks; remove the default
'default-secret-change-me' (and avoid a default URL) and instead make the app
fail fast when those environment variables are missing by validating
process.env.RECOVERY_WEBHOOK_SECRET (and process.env.N8N_WEBHOOK_URL) at startup
in src/main/config.ts; if either is undefined or empty, throw a clear error (or
call process.exit) so the service cannot start with a predictable secret or URL.
In `@src/main/ipc-handlers.ts`:
- Around line 98-103: The 'admin-change-password' IPC handler lacks the same
admin-session guard used elsewhere; update the ipcMain.handle for the
'admin-change-password' channel to call the existing requireAdmin guard at the
start (the same pattern other admin mutations use) before invoking
changePassword(currentPassword, newPassword), so the handler rejects requests
without a valid admin session; reference the ipcMain.handle entry for
'admin-change-password', the requireAdmin guard function, and the changePassword
function and ensure the handler returns the same { success: true } response only
after requireAdmin passes.
In `@src/main/main.ts`:
- Around line 49-64: The delayed admin-status DB query inside the setTimeout
callback (which calls isAdminSetup(), accesses db and uses
db.prepare(...).get(...)) must be wrapped in a try/catch so any runtime or
schema errors don't propagate and crash the main process; modify the setTimeout
callback to enclose all DB access and logging in a try block and on error catch
and call console.warn (or processLogger.warn if available) with a clear message
and the error, and ensure the function degrades gracefully (e.g., treat
adminCount as 0 and setupComplete as false) instead of letting the exception
escape.
In `@src/renderer/components/Admin/AdminLoginModal.tsx`:
- Around line 5-12: AdminLoginModal currently declares onSuccess?: () => void
and drops result.sessionToken; change AdminLoginModalProps so onSuccess is typed
as onSuccess?: (token: string) => void and update the AdminLoginModal
implementation (the success path that handles result.sessionToken, referenced as
result.sessionToken around the success handler lines ~51-55) to call
onSuccess(result.sessionToken) when present instead of calling it with no args;
ensure any internal invocations or passed callbacks to onSuccess in
AdminLoginModal are updated to accept and forward the token.
In `@src/renderer/components/Admin/AdminRegisterModal.tsx`:
- Around line 5-8: The AdminRegisterModal props currently allow onComplete to be
called on cancel/back and without the recovery key; update the
AdminRegisterModalProps so onComplete is typed as onComplete: (recoveryKey:
string) => void, remove any calls to onComplete from handleClose or cancel/back
handlers, and ensure onComplete is invoked only once on the successful
registration/creation path where the generated recoveryKey is available (call
onComplete(recoveryKey) in the success branch of the submit/create function and
do not call it in cancellation or close flows); update any related handlers
referenced in the component (handleClose, submit/create success handler) to
follow this contract.
In `@src/renderer/components/Admin/RecoveryModal.tsx`:
- Around line 73-87: The direct-key validation currently only gates the UI in
handleValidateDirectKey by calling authValidateRecoveryKey(plaintextKey) and
then advancing to setStep('password-reset') without saving the validated
credential, causing authPerformRecovery(recoveryKeyJson || '', newPassword) to
send an empty string; persist the validated key/token (e.g., save plaintextKey
or the returned token from authValidateRecoveryKey) into component state (add a
state var like validatedRecoveryKey) when result.valid is true and then use that
state value instead of recoveryKeyJson (or '' fallback) when calling
authPerformRecovery; apply the same persistence/fallback fix to the other
direct-key path mentioned around the other block (lines ~157-163) so both flows
supply the validated credential to authPerformRecovery.
- Around line 101-107: The email recovery flow stops at setting emailSent=true
and never advances the modal step; update the success path in handleRequestEmail
(after await window.electronAPI.authRequestRecovery()) to also advance the modal
by calling the step setter to 'password-reset' (e.g., setStep('password-reset'))
in addition to setEmailSent(true) and setIsLoading(false); apply the same change
to the analogous handler used in the other recovery path (the similar block
around the other request handler) so both paths move the UI to the
password-reset step on success.
In `@src/renderer/pages/Settings.tsx`:
- Around line 13-129: RemoveAdminModal is defined inside the Settings page
instead of living under the Admin components folder; extract the component into
its own file and update exports/imports to restore the barrel pattern. Create
src/renderer/components/Admin/RemoveAdminModal.tsx containing the interface
RemoveAdminModalProps and the RemoveAdminModal function (preserving
handleConfirm, handleClose and state logic), export it as the default export,
update src/renderer/pages/Settings.tsx to import RemoveAdminModal from the Admin
folder instead of declaring it inline, and add an export in
src/renderer/components/Admin/index.ts (barrel) to re-export RemoveAdminModal so
other modules can import via the existing Admin barrel.
In `@src/renderer/store/useAuthStore.ts`:
- Around line 48-57: refreshStatus currently updates isAuthenticated but leaves
an old sessionToken in the store; modify refreshStatus (in useAuthStore) to
clear the in-memory sessionToken when the auth status response indicates not
authenticated (result.isAuthenticated === false) so stale tokens cannot be
reused—update the set call to set sessionToken: null (or remove it) in the
branch where you set isAuthenticated:false and ensure any existing
error/hasLoaded/isLoading state updates remain consistent.
---
Outside diff comments:
In `@src/main/generated-config.ts`:
- Around line 1-20: Remove the committed generated config and stop tracking the
auto-generated constants: delete the committed file that declares CONFIG with
RECOVERY_WEBHOOK_URL and RECOVERY_WEBHOOK_SECRET from the repo, remove it from
git tracking (git rm --cached), add its filename pattern to .gitignore so it is
never committed again, and ensure the CI/build pipeline writes the file at build
time so the runtime validation block that checks CONFIG.RECOVERY_WEBHOOK_URL and
CONFIG.RECOVERY_WEBHOOK_SECRET remains valid.
---
Minor comments:
In `@AUTH.md`:
- Line 133: Update the contradictory description in AUTH.md so the recovery key
length matches §3.3 and the validation regex: change the phrase "32-character
hexadecimal recovery key" to "64-character hexadecimal (32‑byte) recovery key"
or "32‑byte (64‑character) hexadecimal recovery key" and ensure any nearby
renderer hint (`64 karakter heksadesimal`) is consistent with this wording so
users and validation align.
In `@src/main/auth.test.ts`:
- Around line 279-290: The test currently only checks a fresh session expiry;
change it to capture the initial expiry after login (use loginAdmin to get
sessionToken and read db._seeds['session_expiry']), advance fake timers by a
period (e.g., 5 minutes), call verifySession(token, 1) to trigger extension,
then read the updated db._seeds['session_expiry'] and assert the new expiry is
strictly greater than the captured initial expiry; ensure you use the same token
and user id (verifySession and loginAdmin) and restore timers if you mock them.
In `@src/main/auth.ts`:
- Around line 444-448: The current falsy guard `if (webContentsId)` treats 0 as
absent and silently returns a sessionToken when `webContentsId` is actually
undefined, producing unusable tokens for `verifySession`/`requireAdmin`; change
the guard to an explicit null/undefined check (e.g., `if (webContentsId !==
undefined && webContentsId !== null)`) so numeric 0 is accepted, and when
`webContentsId` is missing explicitly fail instead of returning a token (throw
or return an error) so callers receive a clear failure rather than a silent
unusable `sessionToken`; reference `webContentsId`, `activeSessions`, and
`sessionToken` to locate and update the logic.
In `@src/main/key-management.ts`:
- Around line 115-133: getOrCreateDeviceUUID currently does a SELECT then
INSERT, which causes a race when two callers run concurrently; change it to
perform an atomic upsert/read inside a transaction or a single upsert statement
and then SELECT the value to guarantee determinism: wrap the logic around
db.prepare(...) calls (the SELECT 'SELECT value FROM test_config WHERE key = ?'
and the INSERT 'INSERT INTO test_config (key, value) VALUES (?, ?)') in a
transaction or replace them with a single UPSERT (INSERT ... ON CONFLICT DO
UPDATE/DO NOTHING) and then fetch the row to return the stored UUID; ensure the
function still returns the existing UUID when present and handles errors the
same way.
In `@src/renderer/pages/Settings.tsx`:
- Around line 28-123: Replace hardcoded UI strings with translation keys: change
the literal 'Password is required' used in handleConfirm (setError call), the
input placeholder "Enter current password" on the remove-admin-password input,
and the loading label 'Removing...' in the confirm button to use t('...') calls;
add matching translation keys (for example settings.passwordRequired,
settings.removeAdminPlaceholder, settings.removing) to both English and
Indonesian locale files so the new t(...) lookups return localized text.
In `@src/renderer/store/useAuthStore.test.ts`:
- Around line 39-47: The test helper resetStore currently calls
useAuthStore.setState but omits the hasLoaded property, which can cause state
leakage between tests; update resetStore to include hasLoaded: false in the
object passed to useAuthStore.setState (or alternatively spread the store's
initial state) so that hasLoaded is always reset; locate the resetStore function
in useAuthStore.test.ts and modify the setState payload to explicitly set
hasLoaded: false (or use the store's initialState spread) to ensure clean test
isolation.
In `@src/renderer/types/electronAPI.ts`:
- Line 85: authRequestRecovery currently takes no email which contradicts
AUTH.md §2.2 and allows triggering recovery without proving knowledge of the
registered address; change the renderer API signature authRequestRecovery to
accept an email parameter (authRequestRecovery: (email: string) => Promise<{
success: boolean; message: string }>) and in the implementation (the function
that sends recovery emails) look up the stored email from the DB and perform a
case-insensitive comparison (e.g., normalize both to lower-case) before sending
any outbound recovery email, returning a suitable success/failure message;
alternatively, if you intend the no-input flow, update AUTH.md to remove the
"enter your registered email" step and document the new threat model.
---
Nitpick comments:
In `@src/locales/id/translation.json`:
- Line 280: Update the translation value for the JSON key where "title":
"Otentikasi Admin" to use the preferred spelling "Autentikasi Admin"; locate the
entry in src/locales/id/translation.json with the "title" key currently set to
"Otentikasi Admin" and replace "Otentikasi" with "Autentikasi" to match
KBBI/PUEBI and the app's consistent spelling.
In `@src/main/auth.ts`:
- Around line 107-108: The code uses Date.now() for in-process timing (e.g.,
computing remaining = Math.ceil((lockoutUntil - Date.now()) / 1000)) which
violates the guideline; change in-memory expiry/lockout logic to use
process.hrtime.bigint() (store in-memory expiry times as BigInt nanoseconds) and
compute remaining seconds via bigint arithmetic (e.g., (lockoutUntilNs -
process.hrtime.bigint()) / 1_000_000_000n, then convert to Number and Math.ceil
if needed). Update all in-memory checks referencing lockoutUntil and other
in-process expiries in auth.ts (the same pattern at the other identified sites)
to use the BigInt hrtime values and only keep Date.now()/epoch seconds for
DB-persisted fields (lockout_until, JWT iat/exp).
In `@src/main/key-management.ts`:
- Around line 52-74: encryptWithLMK and decryptWithLMK call getOrCreateLMK() on
every invocation causing expensive keychain calls; add a module-level cache
(e.g., lmkHexCache or lmkPromiseCache) to store the LMK hex (or the ongoing
promise) so subsequent calls reuse it instead of calling keytar each time.
Update both encryptWithLMK and decryptWithLMK to check the cache first, set the
cache when getOrCreateLMK() resolves, and consider caching the promise to avoid
duplicate concurrent getOrCreateLMK() calls; keep the cache immutable for
process lifetime and use the cached value (or awaited promise) when deriving the
key buffer in createCipheriv/createDecipheriv.
In `@src/renderer/store/useNavigation.ts`:
- Around line 23-29: The publicPages lookup is typed as PublicPage[] which
includes null and forces an unnecessary cast in setPage; change the lookup to a
non-null literal tuple (e.g. const publicPages = ['home','about','test'] as
const) or a readonly array of the concrete page string literals, then use that
in setPage (remove the `as PublicPage` cast) and keep assigning
lastVisitedPublicPage when publicPages.includes(page as unknown as typeof
publicPages[number]) is true — better: narrow page first to exclude null (e.g.
if (page && publicPages.includes(page as any))) or use a typed guard so setPage,
publicPages, PublicPage and lastVisitedPublicPage align without casts.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 631b62dc-f263-4145-9de5-3084e4de2604
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (41)
.env.example.github/actions/electron-build-common/action.yml.gitignoreAGENTS.mdAUTH.mdpackage.jsonsrc/locales/en/common.jsonsrc/locales/en/translation.jsonsrc/locales/id/common.jsonsrc/locales/id/translation.jsonsrc/main/AGENTS.mdsrc/main/auth-rate-limit.test.tssrc/main/auth.test.tssrc/main/auth.tssrc/main/config.tssrc/main/generated-config.tssrc/main/ipc-handlers.tssrc/main/key-management.tssrc/main/main.tssrc/preload/AGENTS.mdsrc/preload/preload.tssrc/renderer/App.tsxsrc/renderer/TestScreen.tsxsrc/renderer/components/AGENTS.mdsrc/renderer/components/Admin/AdminLoginModal.tsxsrc/renderer/components/Admin/AdminRegisterModal.tsxsrc/renderer/components/Admin/ChangePasswordModal.tsxsrc/renderer/components/Admin/RecoveryModal.tsxsrc/renderer/components/Admin/index.tssrc/renderer/components/Sidebar.tsxsrc/renderer/hooks/index.tssrc/renderer/hooks/useAuthGuard.tssrc/renderer/hooks/useIdleTimer.tssrc/renderer/main.tsxsrc/renderer/pages/DataManagement.tsxsrc/renderer/pages/Settings.tsxsrc/renderer/store/index.tssrc/renderer/store/useAuthStore.test.tssrc/renderer/store/useAuthStore.tssrc/renderer/store/useNavigation.tssrc/renderer/types/electronAPI.ts
Eliminate non-null assertions, async/await misuse, JSX nesting issues, autoFocus props, void operator misuse, and missing JSDoc comments to improve code quality and pass ESLint with zero errors. All 124 tests pass.
- implement constant-time string comparison to prevent timing side-channel attacks - add type-only imports for better tree-shaking - refactor tests to use shared constants for DB keys and values - update event handlers to avoid promise-returning function warnings - fix documentation to add escape hatch for auth architecture rule - fix documentation to expand AUTH acronym on first use
Email recovery required RECOVERY_WEBHOOK_URL/SECRET environment variables which blocked app startup when unset. Simplify recovery to direct key-only flow to eliminate external service dependency and enable out-of-the-box operation without configuration
|
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| JavaScript | |
May 11, 2026 10:48a.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/main/auth.ts (1)
121-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
bcrypt.compareSyncstill blocks the Electron main process.Three sync compares remain at Lines 121, 228, 447 — each holds the event loop for ~300–500 ms at cost 12, freezing IPC and window management. The hashing call sites have already moved to
await bcrypt.hash; these three should follow suit.loginAdminwould also needasync(orawaitinside its current Promise-returning shape) — see the snippet below.🔧 Proposed fix
- const isValid = bcrypt.compareSync(password, hashRow.value); + const isValid = await bcrypt.compare(password, hashRow.value); ... - if (!hashRow || !bcrypt.compareSync(password, hashRow.value)) { + if (!hashRow || !(await bcrypt.compare(password, hashRow.value))) { ... - if (!hashRow || !bcrypt.compareSync(currentPassword, hashRow.value)) { + if (!hashRow || !(await bcrypt.compare(currentPassword, hashRow.value))) {Note:
loginAdminanddeleteAdminwill need to be converted toasync(currently returning manually-constructed Promises) to await the comparison.Also applies to: 228-228, 447-447
🤖 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 `@src/main/auth.ts` at line 121, Replace blocking bcrypt.compareSync calls with the async bcrypt.compare and make the surrounding functions async so you can await the comparisons: locate the three uses of bcrypt.compareSync (e.g., in loginAdmin and deleteAdmin and the third compare site), change the functions (loginAdmin, deleteAdmin, and the function containing the third compare) from manually-constructed Promise patterns to async functions, await bcrypt.compare(password, hashRow.value) (or the appropriate args), and return/throw as before; update any callers to handle the returned Promise if needed.AUTH.md (1)
286-289:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix contradictory lost-key recovery instructions in Troubleshooting.
Line 286-Line 289 conflicts with Line 208-Line 212. “Uninstall/reinstall” plus “test data is preserved” is incorrect for the documented LMK/database reset model and can mislead users during lockout recovery.
Suggested doc patch
-1. Uninstall and reinstall F.O.C.U.S. -2. Register a new admin account on first run. -3. The database test data is preserved; only admin credentials are reset. +1. Delete the application's SQLite database in the Electron `userData` directory. +2. Delete the OS keychain entry for service `focus-auth` and account `local-master-key`. +3. Relaunch and register a new admin account. +4. Note: deleting the database removes test results; deleting only the keychain entry does not fully reset auth.🤖 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 `@AUTH.md` around lines 286 - 289, The Troubleshooting block containing the three lines "Uninstall and reinstall F.O.C.U.S.", "Register a new admin account on first run.", and "The database test data is preserved; only admin credentials are reset." contradicts the earlier lost-key/LMK database reset model; update AUTH.md to remove or reword those lines so they match the documented LMK/database reset behavior in the earlier troubleshooting section (the lost-key recovery instructions), explicitly stating whether uninstall/reinstall preserves or wipes test data and how admin credentials are recovered, and ensure the phrasing appears under the same "Troubleshooting" / "lost-key recovery" heading to avoid duplication or conflict.
🧹 Nitpick comments (6)
src/locales/en/translation.json (1)
279-368: 💤 Low valueLGTM — admin namespace looks consistent.
Strings are organized logically and match the AI summary's modals. One drive-by:
RemoveAdminModalreusesadmin.changePassword.error.requiredfor a missing remove-admin password — semantically that key belongs to the change-password flow, so consider a dedicatedadmin.remove.error.required(or similar) later for clarity.🤖 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 `@src/locales/en/translation.json` around lines 279 - 368, The review notes that RemoveAdminModal currently reuses admin.changePassword.error.required which is semantically incorrect; add a dedicated key (e.g., admin.remove.error.required) to translation.json and update the RemoveAdminModal to reference that new key instead of admin.changePassword.error.required so remove-admin errors are isolated and clear.src/main/auth.test.ts (2)
60-75: 💤 Low value
deviceUuidQueryCountbranch is dead-on-arrival.If
seeds[key] !== undefinedyou return immediately; otherwise on the first query you return undefined. Theif (deviceUuidQueryCount === 1) return undefined;branch effectively duplicates the defaultreturn seeds[key] !== undefined ? ... : undefined;that follows. Either delete the counter or replace this whole block with a single early-return for clarity.🤖 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 `@src/main/auth.test.ts` around lines 60 - 75, The deviceUuidQueryCount branch in statementMock.get is redundant and should be removed or simplified: update the get method (in the statementMock object) to avoid the dead deviceUuidQueryCount logic and instead just early-return when seeds[DB_KEYS.DEVICE_UUID] is present, otherwise return undefined for missing keys; remove the deviceUuidQueryCount variable and the special-case branch so get(key) simply checks seeds[key] and returns { value: seeds[key] } when defined or undefined when not.
309-332: 💤 Low valueSubtle: this test passes despite mixing two clocks.
verifySessionchecks expiry viaprocess.hrtime.bigint(), which Vitest fake timers do not mock. The test only advancesDate.now(), so the in-memory hrtime expiry never actually moves; the assertion succeeds only becausenewExpiryreads from the DB write (which usesDate.now()). This means the test isn't really exercising "extension after 5 minutes of real time" — consider adding a parallel test that pushes the in-memory expiry close toprocess.hrtime.bigint()to truly validate the extension path.🤖 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 `@src/main/auth.test.ts` around lines 309 - 332, The test mixes Date.now() and process.hrtime.bigint() clocks so it falsely passes; update the test for verifySession to also move or align the hrtime clock used by verifySession: either stub/spy process.hrtime.bigint() to return an advanced BigInt (so verifySession sees 5 minutes of elapsed hrtime) or set the in-memory session expiry (the value verifySession compares against) to a value just before now so the verifySession path that extends expiry is exercised; reference verifySession, loginAdmin, db._seeds and DB_KEYS.SESSION_EXPIRY when making the change so you update the same session token and expiry the test reads.src/main/auth.ts (1)
8-8: 💤 Low valueUse
import typefor the type-only Electron import.Codacy flagged this —
IpcMainInvokeEventis referenced only as a type at line 477, so this can be a type-only import to avoid pulling Electron into emitted JS.-import { IpcMainInvokeEvent } from 'electron'; +import type { IpcMainInvokeEvent } from 'electron';🤖 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 `@src/main/auth.ts` at line 8, Change the top-level Electron import to a type-only import so the runtime build doesn't pull Electron into emitted JS: replace the regular import of IpcMainInvokeEvent with a type import (i.e., use "import type { IpcMainInvokeEvent } from 'electron'") where IpcMainInvokeEvent is referenced (notably the handler using IpcMainInvokeEvent around line 477) so only the type is imported at compile time.src/renderer/components/Admin/RemoveAdminModal.tsx (1)
49-126: 💤 Low valueLGTM — destructive modal looks safe by default.
Backdrop click doesn't dismiss (good for destructive UX), confirm is gated on a non-empty password, error/loading states are clean, and ARIA attributes (
role="dialog",aria-modal,aria-labelledby) are wired up. Nit: no Escape-key handler or focus trap, so keyboard users can tab out of the modal — worth aligning across the admin modals in a follow-up.🤖 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 `@src/renderer/components/Admin/RemoveAdminModal.tsx` around lines 49 - 126, RemoveAdminModal lacks keyboard support: add an Escape-key handler to call handleClose and implement a focus trap so keyboard users cannot tab out of the modal; update the RemoveAdminModal component to attach a keydown listener (listening for Escape) that invokes handleClose and to trap focus inside the modal root (use the same focus-trap implementation you use elsewhere in admin modals or a lightweight approach with sentinels and focus management around the modal container), and ensure cleanup on unmount so isLoading/disabled states still work with handleConfirm and handleClose.src/test/constants.ts (1)
34-38: 💤 Low valueRemove unused constants
TEST_PASSWORD_HASHandDETERMINISTIC_HASH.These are exported but not used anywhere in the codebase. Removing them eliminates dead code.
🤖 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 `@src/test/constants.ts` around lines 34 - 38, Remove the unused exported constants TEST_PASSWORD_HASH and DETERMINISTIC_HASH from the module: delete their declarations and any related export so they are no longer part of the public API; if other modules require these values, replace import sites with the appropriate test fixture or mock provider instead of re-exporting these constants. Ensure no runtime references remain (search for TEST_PASSWORD_HASH and DETERMINISTIC_HASH) and run tests to confirm nothing breaks.
🤖 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 `@src/main/auth-rate-limit.test.ts`:
- Around line 56-111: The createMockDb helper is duplicated with drift between
tests: extract a single shared implementation to src/test/helpers/mockDb.ts and
update both auth.test.ts and auth-rate-limit.test.ts to import it; ensure the
unified createMockDb preserves the expected behavior by including the DELETE
branch, making transaction<T>(fn: () => T) return a wrapper that calls fn()
(matching the other file), and keeping statementMock.run, prepare, seeds, and
the device-UUID-first-query logic intact so both suites use the same
deterministic mocks.
In `@src/main/auth.ts`:
- Around line 344-432: The guard for webContentsId is checked after DB mutations
in performRecovery, which can leave credentials rotated if the caller lacks a
valid webContentsId; move the check `if (webContentsId === undefined)` to the
top of performRecovery (before hashing, generating new recovery key, and the
db.transaction/upsert block) and remove the unreachable `=== null` branch per
the function signature; ensure activeSessions.set and session expiry DB write
still run only after the validation passes so no DB changes occur when
webContentsId is missing.
In `@src/main/ipc-handlers.ts`:
- Around line 64-66: The IPC handler for 'admin-register' currently allows
unauthenticated calls to overwrite an existing admin; update the
ipcMain.handle('admin-register', ...) to first call isAdminSetup() and
reject/throw an error (or return a failure) if an admin is already set, and also
add the same guard inside registerAdmin in src/main/auth.ts so registerAdmin
checks isAdminSetup() and refuses to UPSERT when setup is complete; reference
the ipc handler ('admin-register'), the registerAdmin function, and the
isAdminSetup() helper when making these changes.
In `@src/renderer/components/Admin/AdminRegisterModal.tsx`:
- Around line 17-113: The AdminRegisterModal currently contains auth
orchestration (validation, IPC calls, login side effects) inside functions like
handleRegister, handleContinue, validateEmail, resetState and handleCopyKey;
extract those into a new hook or into the existing Zustand store (e.g., create
useAuthRegister hook or add methods in src/renderer/store/index.ts such as
registerAdmin, loginWithPassword, validateEmail, copyRecoveryKey,
resetRegistrationState) and replace the modal handlers with calls to that
hook/store while keeping AdminRegisterModal purely presentational (props for
state and callbacks only) and delegating side effects and IPC
(window.electronAPI.authRegister/authLogin) to the store/hook.
In `@src/renderer/components/Admin/RecoveryModal.tsx`:
- Around line 17-123: The component currently contains recovery workflow state
(plaintextKey, validatedRecoveryKey, newPassword, confirmPassword, step, error,
isLoading) and IPC side effects (window.electronAPI.authValidateRecoveryKey,
window.electronAPI.authPerformRecovery) inside RecoveryModal; extract this logic
into a dedicated hook or Zustand store (per renderer store rules) that exposes
state, setters, and actions (e.g., validatePlaintextKey, validateDirectKey
action replacing handleValidateDirectKey, performRecovery action replacing
handleResetPassword, and resetState action), move constantTimeEquals usage into
the same layer, and have RecoveryModal become purely presentational by consuming
the store/hook actions and derived state (and calling login via an exposed
action or callback) while keeping only UI rendering and onClose handling.
---
Duplicate comments:
In `@AUTH.md`:
- Around line 286-289: The Troubleshooting block containing the three lines
"Uninstall and reinstall F.O.C.U.S.", "Register a new admin account on first
run.", and "The database test data is preserved; only admin credentials are
reset." contradicts the earlier lost-key/LMK database reset model; update
AUTH.md to remove or reword those lines so they match the documented
LMK/database reset behavior in the earlier troubleshooting section (the lost-key
recovery instructions), explicitly stating whether uninstall/reinstall preserves
or wipes test data and how admin credentials are recovered, and ensure the
phrasing appears under the same "Troubleshooting" / "lost-key recovery" heading
to avoid duplication or conflict.
In `@src/main/auth.ts`:
- Line 121: Replace blocking bcrypt.compareSync calls with the async
bcrypt.compare and make the surrounding functions async so you can await the
comparisons: locate the three uses of bcrypt.compareSync (e.g., in loginAdmin
and deleteAdmin and the third compare site), change the functions (loginAdmin,
deleteAdmin, and the function containing the third compare) from
manually-constructed Promise patterns to async functions, await
bcrypt.compare(password, hashRow.value) (or the appropriate args), and
return/throw as before; update any callers to handle the returned Promise if
needed.
---
Nitpick comments:
In `@src/locales/en/translation.json`:
- Around line 279-368: The review notes that RemoveAdminModal currently reuses
admin.changePassword.error.required which is semantically incorrect; add a
dedicated key (e.g., admin.remove.error.required) to translation.json and update
the RemoveAdminModal to reference that new key instead of
admin.changePassword.error.required so remove-admin errors are isolated and
clear.
In `@src/main/auth.test.ts`:
- Around line 60-75: The deviceUuidQueryCount branch in statementMock.get is
redundant and should be removed or simplified: update the get method (in the
statementMock object) to avoid the dead deviceUuidQueryCount logic and instead
just early-return when seeds[DB_KEYS.DEVICE_UUID] is present, otherwise return
undefined for missing keys; remove the deviceUuidQueryCount variable and the
special-case branch so get(key) simply checks seeds[key] and returns { value:
seeds[key] } when defined or undefined when not.
- Around line 309-332: The test mixes Date.now() and process.hrtime.bigint()
clocks so it falsely passes; update the test for verifySession to also move or
align the hrtime clock used by verifySession: either stub/spy
process.hrtime.bigint() to return an advanced BigInt (so verifySession sees 5
minutes of elapsed hrtime) or set the in-memory session expiry (the value
verifySession compares against) to a value just before now so the verifySession
path that extends expiry is exercised; reference verifySession, loginAdmin,
db._seeds and DB_KEYS.SESSION_EXPIRY when making the change so you update the
same session token and expiry the test reads.
In `@src/main/auth.ts`:
- Line 8: Change the top-level Electron import to a type-only import so the
runtime build doesn't pull Electron into emitted JS: replace the regular import
of IpcMainInvokeEvent with a type import (i.e., use "import type {
IpcMainInvokeEvent } from 'electron'") where IpcMainInvokeEvent is referenced
(notably the handler using IpcMainInvokeEvent around line 477) so only the type
is imported at compile time.
In `@src/renderer/components/Admin/RemoveAdminModal.tsx`:
- Around line 49-126: RemoveAdminModal lacks keyboard support: add an Escape-key
handler to call handleClose and implement a focus trap so keyboard users cannot
tab out of the modal; update the RemoveAdminModal component to attach a keydown
listener (listening for Escape) that invokes handleClose and to trap focus
inside the modal root (use the same focus-trap implementation you use elsewhere
in admin modals or a lightweight approach with sentinels and focus management
around the modal container), and ensure cleanup on unmount so isLoading/disabled
states still work with handleConfirm and handleClose.
In `@src/test/constants.ts`:
- Around line 34-38: Remove the unused exported constants TEST_PASSWORD_HASH and
DETERMINISTIC_HASH from the module: delete their declarations and any related
export so they are no longer part of the public API; if other modules require
these values, replace import sites with the appropriate test fixture or mock
provider instead of re-exporting these constants. Ensure no runtime references
remain (search for TEST_PASSWORD_HASH and DETERMINISTIC_HASH) and run tests to
confirm nothing breaks.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8fd0937d-1b21-4ee0-9d3f-ebb47966bc8c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
.env.example.gitignoreAGENTS.mdAUTH.mdpackage.jsonsrc/locales/en/translation.jsonsrc/locales/id/translation.jsonsrc/main/auth-rate-limit.test.tssrc/main/auth.test.tssrc/main/auth.tssrc/main/ipc-handlers.tssrc/main/key-management.tssrc/main/main.tssrc/preload/preload.tssrc/renderer/TestScreen.tsxsrc/renderer/components/Admin/AdminLoginModal.tsxsrc/renderer/components/Admin/AdminRegisterModal.tsxsrc/renderer/components/Admin/ChangePasswordModal.tsxsrc/renderer/components/Admin/RecoveryModal.tsxsrc/renderer/components/Admin/RemoveAdminModal.tsxsrc/renderer/components/Admin/index.tssrc/renderer/components/EmailCaptureForm.tsxsrc/renderer/components/LanguageSwitcher.tsxsrc/renderer/components/Sidebar.tsxsrc/renderer/hooks/useIdleTimer.tssrc/renderer/pages/About.tsxsrc/renderer/pages/DataManagement.tsxsrc/renderer/pages/Home.tsxsrc/renderer/pages/Settings.tsxsrc/renderer/store/useAuthStore.test.tssrc/renderer/store/useAuthStore.tssrc/renderer/types/electronAPI.tssrc/renderer/types/trial.tssrc/renderer/utils/constantTime.tssrc/test/constants.ts
✅ Files skipped from review due to trivial changes (7)
- src/renderer/types/trial.ts
- src/renderer/components/LanguageSwitcher.tsx
- src/renderer/pages/Home.tsx
- src/renderer/components/EmailCaptureForm.tsx
- src/renderer/pages/About.tsx
- .gitignore
- src/renderer/components/Admin/index.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- .env.example
- src/renderer/hooks/useIdleTimer.ts
- src/renderer/store/useAuthStore.ts
- src/preload/preload.ts
- src/renderer/components/Admin/AdminLoginModal.tsx
- package.json
- src/renderer/TestScreen.tsx
- src/renderer/types/electronAPI.ts
- src/main/main.ts
- src/locales/id/translation.json
- src/renderer/pages/Settings.tsx
- src/renderer/components/Sidebar.tsx
- src/renderer/pages/DataManagement.tsx
- src/renderer/components/Admin/ChangePasswordModal.tsx
- src/renderer/store/useAuthStore.test.ts
- src/main/key-management.ts
Comment on lines
+64
to
+66
| ipcMain.handle('admin-register', async (_event, email: string, password: string) => { | ||
| return await registerAdmin(email, password); | ||
| }); |
Contributor
There was a problem hiding this comment.
admin-register can silently overwrite an existing admin.
registerAdmin UPSERTs every auth key without first checking isAdminSetup(), and this IPC handler is unguarded (correctly, since it's needed for first-time setup). The result: any unauthenticated renderer can call admin-register after setup and replace the password hash + recovery key, locking the real admin out. Guard the handler with an isAdminSetup() check.
🔧 Proposed fix
-ipcMain.handle('admin-register', async (_event, email: string, password: string) => {
- return await registerAdmin(email, password);
-});
+ipcMain.handle('admin-register', async (_event, email: string, password: string) => {
+ if (isAdminSetup()) {
+ throw new Error('Admin is already registered');
+ }
+ return await registerAdmin(email, password);
+});Belt-and-suspenders: replicate the same check inside registerAdmin in src/main/auth.ts so the invariant holds for any future caller.
🤖 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 `@src/main/ipc-handlers.ts` around lines 64 - 66, The IPC handler for
'admin-register' currently allows unauthenticated calls to overwrite an existing
admin; update the ipcMain.handle('admin-register', ...) to first call
isAdminSetup() and reject/throw an error (or return a failure) if an admin is
already set, and also add the same guard inside registerAdmin in
src/main/auth.ts so registerAdmin checks isAdminSetup() and refuses to UPSERT
when setup is complete; reference the ipc handler ('admin-register'), the
registerAdmin function, and the isAdminSetup() helper when making these changes.
Comment on lines
+17
to
+123
| export const RecoveryModal: FC<RecoveryModalProps> = ({ isOpen, onClose }) => { | ||
| const { t } = useTranslation('translation'); | ||
| const login = useAuthStore((state) => state.login); | ||
|
|
||
| // Direct recovery state | ||
| const [plaintextKey, setPlaintextKey] = useState(''); | ||
| const [validatedRecoveryKey, setValidatedRecoveryKey] = useState(''); | ||
|
|
||
| // Password reset state | ||
| const [newPassword, setNewPassword] = useState(''); | ||
| const [confirmPassword, setConfirmPassword] = useState(''); | ||
|
|
||
| // UI state | ||
| const [step, setStep] = useState<RecoveryStep>('validate-key'); | ||
| const [error, setError] = useState(''); | ||
| const [isLoading, setIsLoading] = useState(false); | ||
|
|
||
| /** Resets all form fields to their initial empty state. */ | ||
| const resetState = () => { | ||
| setPlaintextKey(''); | ||
| setValidatedRecoveryKey(''); | ||
| setNewPassword(''); | ||
| setConfirmPassword(''); | ||
| setStep('validate-key'); | ||
| setError(''); | ||
| setIsLoading(false); | ||
| }; | ||
|
|
||
| /** Handles closing the recovery modal. */ | ||
| const handleClose = () => { | ||
| resetState(); | ||
| onClose?.(); | ||
| }; | ||
|
|
||
| // --- Direct recovery: validate plaintext key --- | ||
|
|
||
| const validatePlaintextKey = (value: string): boolean => { | ||
| return /^[a-fA-F0-9]{64}$/.test(value); | ||
| }; | ||
|
|
||
| /** Handles validation of the direct recovery key input. */ | ||
| const handleValidateDirectKey = async (e: React.FormEvent) => { | ||
| e.preventDefault(); | ||
| setError(''); | ||
|
|
||
| if (!validatePlaintextKey(plaintextKey)) { | ||
| setError(t('admin.recovery.invalidKey')); | ||
| return; | ||
| } | ||
|
|
||
| setIsLoading(true); | ||
|
|
||
| try { | ||
| const result = await window.electronAPI.authValidateRecoveryKey(plaintextKey); | ||
| if (result.valid) { | ||
| setValidatedRecoveryKey(plaintextKey); | ||
| setStep('reset-password'); | ||
| } else { | ||
| setError(t('admin.recovery.invalidKey')); | ||
| } | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : t('admin.recovery.direct.error.failed'); | ||
| setError(message); | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }; | ||
|
|
||
| // --- Password reset --- | ||
|
|
||
| /** Handles password reset form submission after recovery key validation. */ | ||
| const handleResetPassword = async (e: React.FormEvent) => { | ||
| e.preventDefault(); | ||
| setError(''); | ||
|
|
||
| // Validate passwords | ||
| if (!newPassword || !confirmPassword) { | ||
| setError(t('admin.recovery.step2.error.required')); | ||
| return; | ||
| } | ||
|
|
||
| if (newPassword.length < 8) { | ||
| setError(t('admin.recovery.step2.error.newPasswordLength')); | ||
| return; | ||
| } | ||
|
|
||
| if (!constantTimeEquals(newPassword, confirmPassword)) { | ||
| setError(t('admin.recovery.step2.error.passwordsMatch')); | ||
| return; | ||
| } | ||
|
|
||
| setIsLoading(true); | ||
|
|
||
| try { | ||
| const result = await window.electronAPI.authPerformRecovery( | ||
| validatedRecoveryKey, | ||
| newPassword | ||
| ); | ||
| login(result.sessionToken); | ||
| handleClose(); | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : t('admin.recovery.step2.error.failed'); | ||
| setError(message); | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Move auth flow state/logic out of the component.
From Line 17 onward, this component owns both recovery workflow state and IPC side effects. That conflicts with the renderer architecture rules and will make auth behavior harder to keep consistent across admin surfaces. Please extract this into a dedicated hook/store layer and keep this component presentational.
As per coding guidelines: “All UI state must live in src/renderer/store/index.ts (Zustand store)” and “Components are presentational only – any state/logic lives in a hook or the Zustand store.”
🤖 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 `@src/renderer/components/Admin/RecoveryModal.tsx` around lines 17 - 123, The
component currently contains recovery workflow state (plaintextKey,
validatedRecoveryKey, newPassword, confirmPassword, step, error, isLoading) and
IPC side effects (window.electronAPI.authValidateRecoveryKey,
window.electronAPI.authPerformRecovery) inside RecoveryModal; extract this logic
into a dedicated hook or Zustand store (per renderer store rules) that exposes
state, setters, and actions (e.g., validatePlaintextKey, validateDirectKey
action replacing handleValidateDirectKey, performRecovery action replacing
handleResetPassword, and resetState action), move constantTimeEquals usage into
the same layer, and have RecoveryModal become purely presentational by consuming
the store/hook actions and derived state (and calling login via an exposed
action or callback) while keeping only UI rendering and onClose handling.
static checks identified issues in auth, admin modals, and preload requiring async bcrypt, JSX extraction, autoFocus removal, void operator fixes, and state bug correction. All tests pass, build succeeds.
…ytar Address multiple linter violations reported in code audit: - arrow functions returning void now use explicit braces to avoid ambiguity - non-null assertions replaced with null-safe alternatives in test files - test mock objects use null-prototype to prevent prototype pollution - dynamic delete operations replaced with undefined assignments - promise-returning calls in void contexts properly handled with void or await - form accessibility: label associated with recovery key input via htmlFor - type-only imports use import type syntax for clarity feat: make keytar service/account configurable via Settings page - new IPC handlers: get-keytar-config, save-keytar-config - key-management reads config from DB with sensible defaults - Settings UI section for keychain credentials with save button - automatic LMK migration when credentials change - backward compatible: defaults preserve existing installations docs: clarify AUTH acronym and improve readability in AGENTS.md
…t modal logic to store Consolidate duplicate mockDb implementations into a single shared helper to reduce maintenance burden and ensure consistent test behavior across auth suites. Remove dead code (deviceUuidQueryCount) and unused constants (TEST_PASSWORD_HASH, DETERMINISTIC_HASH) to improve code clarity. Move all business logic out of AdminRegisterModal and RecoveryModal into the Zustand auth store, making these components purely presentational and following the established component responsibility pattern. This separation improves testability and keeps UI rendering distinct from side effects and state transitions. Harden security by validating webContentsId upfront in performRecovery before any expensive operations or DB mutations occur, and add isAdminSetup guards at both IPC and function layers to prevent overwriting an existing admin account. These layers of defense protect against both accidental misuse and misconfigured clients. Improve accessibility by adding Escape-key handling and a custom focus trap to RemoveAdminModal, ensuring keyboard users can dismiss the modal and remain within the focus cycle while modal is open. Dedicate a dedicated error key (admin.remove.error.required) to keep translation semantics consistent. Align AUTH.md troubleshooting with the documented lost-key recovery flow by explicitly stating that test data is permanently erased during recovery, removing contradictory guidance that claimed data preservation. Fix verifySession test to properly validate expiry extension without mixing Date.now and hrtime with fake timers.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@AUTH.md`:
- Around line 20-21: The docs currently describe an email/N8N recovery flow but
the shipped app implements a renderer recovery UI that requires direct key
entry; update AUTH.md to reflect the actual recovery flow shipped: remove or
clearly mark email/N8N as non-functional in this build, document that recovery
is renderer-only and requires entering the raw recovery key (or importing a key
file) via the renderer recovery UI, and explain any prerequisites (device-bound
encryption and that privileged ops run in the Electron main process) and
expected UX/limitations so readers following "three-layer security model",
"renderer recovery UI", and the recovery sections know the correct steps and
failure modes.
In `@src/main/auth.ts`:
- Around line 67-91: The admin_password_hash is being stored in plaintext
(passwordHash) in test_config; instead encrypt the bcrypt verifier with the
existing LMK flow before persisting and only decrypt it where
comparisons/updates occur: call encryptWithLMK(passwordHash) and store
encryptedPassword.ciphertext, .iv, .tag under dedicated keys (e.g.,
admin_password_ciphertext/admin_password_iv/admin_password_tag) instead of
admin_password_hash, update the db.transaction block that uses upsertStmt and
the functions that perform recovery/password-change to decrypt via
decryptWithLMK when comparing or re-hashing, and remove any other direct writes
of passwordHash (e.g., the other locations mentioned) so all password verifier
storage follows the LMK encryption pattern used for recoveryKey.
- Around line 296-326: validateRecoveryKey currently only checks the provided
recovery key and performRecovery consumes the raw recovery key and new password
in one step; split the flow into two steps: (1) update validateRecoveryKey to
verify the provided recoveryKey using decryptWithLMK and, on success, generate
and store a short-lived reset token (linked to the account) instead of accepting
a new password, returning only validity and the token expiry info; (2) change
performRecovery to accept the reset token plus the new password, validate the
token against the stored token, then re-encrypt stored keys and rotate the
recovery credential; additionally implement per-device failure counting tied to
device UUID (increment failures on invalid recoveryKey or invalid reset token in
validateRecoveryKey and performRecovery respectively, reset counter on success,
and enforce a lockout after 5 failures for 1 minute) and reuse DB entries
already referenced (test_config keys and functions like decryptWithLMK) to
persist tokens, counters, timestamps, and lockout state.
In `@src/main/key-management.ts`:
- Around line 34-64: getOrCreateLMK currently generates a random key and stores
it directly; instead derive the LMK deterministically from OS-native secret
material plus the persistent device UUID so the key is installation-bound.
Modify getOrCreateLMK to: 1) read an OS-secret from keytar (use the same
service/account logic with DEFAULT_KEYTAR_SERVICE/DEFAULT_KEYTAR_ACCOUNT and
DB_KEY_KEYTAR_SERVICE/DB_KEY_KEYTAR_ACCOUNT); 2) if the OS-secret is missing,
generate a high-entropy OS-secret (randomBytes) and store it via
keytar.setPassword; 3) call getOrCreateDeviceUUID() to obtain the persistent
device UUID and use it as the salt; 4) derive a 32-byte LMK using a standard KDF
(HKDF-SHA256 or HMAC-based KDF) from the OS-secret with the device UUID as salt
and return that derived key (do not replace the stored OS-secret with the
derived LMK). Update any code paths that used randomBytes(32).toString('hex') so
they use the derived output instead.
In `@src/renderer/components/Admin/AdminLoginModal.tsx`:
- Around line 32-44: Create a single shared handleClose() that first clears the
component's sensitive local state (reset password and error via setPassword('')
and setError(null/'' as appropriate) or whatever state setters are used) and
then calls onClose?.(); replace direct calls to onClose in handleBackdropClick
and handleKeyDown with handleClose, and update the other exit paths referenced
(the back button handler and the success/submit-complete handler) to call
handleClose as well so every dismiss path clears sensitive state before closing.
In `@src/renderer/components/Admin/AdminRegisterModal.tsx`:
- Around line 37-50: The RecoveryKeyPhase is being returned directly which
bypasses the modal shell and dialog semantics; instead wrap the RecoveryKeyPhase
component in the same overlay/modal container used by the registration step (the
modal backdrop and element with role="dialog" in AdminRegisterModal) so it
renders inside the fixed modal shell and retains focus/aria semantics; update
the branch that returns RecoveryKeyPhase (and the other occurrence later in the
file that mirrors lines ~279-355) to render the overlay/backdrop and the dialog
container and then render <RecoveryKeyPhase .../> inside it, preserving the same
props and event handlers.
In `@src/renderer/components/Admin/RecoveryModal.tsx`:
- Around line 12-16: The RecoveryModal currently implements the direct key flow
using validateRecoveryKey and performRecovery; change it to implement the
required email/reset-token flow by replacing calls to validateRecoveryKey and
performRecovery with requestRecovery for step 1 (trigger email/reset-token) and
completeRecovery for step 2 (submit reset token and new password). Update the
component logic (RecoveryModal) to accept and handle the reset-token
input/state, call requestRecovery when the user submits their email, render the
token/password form after requestRecovery succeeds, and call completeRecovery to
finish recovery; ensure error/success paths and prop/signature usages are
adjusted accordingly where validateRecoveryKey/performRecovery are currently
referenced.
- Around line 35-41: The backdrop dismissal handlers are attached to the inner
panel instead of the overlay; move the click and Escape close logic from the
white card element to the outer overlay div so clicks on the dark backdrop and
pressing Escape close the modal while the panel remains passive. Specifically,
remove any onClick/onKeyDown (or onKeyUp) close handlers from the panel element
in RecoveryModal and attach a click handler on the overlay div that calls the
existing close function (and add a useEffect or overlay-level keydown listener
to handle Escape if currently handled on the panel). Repeat the same change for
the duplicate modal block referenced around the 141-160 region so both overlay
divs manage dismissal and the inner panels have no close handlers.
In `@src/renderer/components/Results/AcsCalculationModal/AcsCalculationModal.tsx`:
- Around line 39-41: Update the misleading JSDoc above handleCancel: change the
text to state that handleCancel handles the native modal close event (e.g.,
Escape key or dialog close), not the modal's close button, and note that the
close button calls onClose directly; edit the comment near the handleCancel
function to reflect this distinction (referencing handleCancel and onClose).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: baca2a91-7992-416c-9ccd-35faf61a1c0e
📒 Files selected for processing (31)
AGENTS.mdAUTH.mdsrc/locales/en/translation.jsonsrc/main/auth-rate-limit.test.tssrc/main/auth.test.tssrc/main/auth.tssrc/main/constants.tssrc/main/ipc-handlers.tssrc/main/key-management.tssrc/preload/preload.tssrc/renderer/App.tsxsrc/renderer/TestScreen.tsxsrc/renderer/components/Admin/AdminLoginModal.tsxsrc/renderer/components/Admin/AdminRegisterModal.tsxsrc/renderer/components/Admin/ChangePasswordModal.tsxsrc/renderer/components/Admin/RecoveryModal.tsxsrc/renderer/components/Admin/RemoveAdminModal.tsxsrc/renderer/components/EmailCaptureForm.tsxsrc/renderer/components/Results/AcsCalculationModal/AcsCalculationModal.tsxsrc/renderer/components/Sidebar.tsxsrc/renderer/hooks/useAuthGuard.tssrc/renderer/hooks/useIdleTimer.tssrc/renderer/hooks/useTestPhase.tssrc/renderer/pages/DataManagement.tsxsrc/renderer/pages/Settings.tsxsrc/renderer/store/useAuthStore.tssrc/renderer/store/useDataManagementStore.tssrc/renderer/store/useNavigation.tssrc/renderer/types/electronAPI.tssrc/test/constants.tssrc/test/helpers/mockDb.ts
✅ Files skipped from review due to trivial changes (1)
- src/renderer/store/useDataManagementStore.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- src/test/constants.ts
- src/renderer/App.tsx
- src/renderer/types/electronAPI.ts
- src/locales/en/translation.json
- src/renderer/components/Sidebar.tsx
- src/preload/preload.ts
- src/renderer/store/useNavigation.ts
- src/renderer/components/EmailCaptureForm.tsx
- src/renderer/hooks/useAuthGuard.ts
- src/renderer/TestScreen.tsx
- src/renderer/pages/Settings.tsx
- src/main/ipc-handlers.ts
Comment on lines
+20
to
+21
| The admin authentication system uses a **three-layer security model** combining password hashing, device-bound encryption, and email-verified recovery. All privileged operations are owned by the Electron main process; the renderer has no direct filesystem or cryptographic access. | ||
|
|
Contributor
There was a problem hiding this comment.
Document the recovery flow that actually ships.
These sections describe email/N8N recovery as the active user path, but this PR’s renderer recovery UI is direct-key-only. As written, users following the docs will hit a flow the app cannot complete.
Also applies to: 141-170, 289-303
🤖 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 `@AUTH.md` around lines 20 - 21, The docs currently describe an email/N8N
recovery flow but the shipped app implements a renderer recovery UI that
requires direct key entry; update AUTH.md to reflect the actual recovery flow
shipped: remove or clearly mark email/N8N as non-functional in this build,
document that recovery is renderer-only and requires entering the raw recovery
key (or importing a key file) via the renderer recovery UI, and explain any
prerequisites (device-bound encryption and that privileged ops run in the
Electron main process) and expected UX/limitations so readers following
"three-layer security model", "renderer recovery UI", and the recovery sections
know the correct steps and failure modes.
Comment on lines
+67
to
+91
| const passwordHash = await bcrypt.hash(password, 12); | ||
| const encryptedEmail = await encryptWithLMK(email); | ||
| const deviceUuid = getOrCreateDeviceUUID(db); | ||
|
|
||
| // Generate 32-byte recovery key | ||
| const recoveryKey = randomBytes(32).toString('hex'); | ||
| const encryptedRecovery = await encryptWithLMK(recoveryKey); | ||
|
|
||
| const upsertStmt = db.prepare(` | ||
| INSERT INTO test_config (key, value) VALUES (?, ?) | ||
| ON CONFLICT(key) DO UPDATE SET value = excluded.value | ||
| `); | ||
|
|
||
| db.transaction(() => { | ||
| upsertStmt.run('admin_password_hash', passwordHash); | ||
| upsertStmt.run('admin_email_ciphertext', encryptedEmail.ciphertext); | ||
| upsertStmt.run('admin_email_iv', encryptedEmail.iv); | ||
| upsertStmt.run('admin_email_tag', encryptedEmail.tag); | ||
| upsertStmt.run('admin_device_uuid', deviceUuid); | ||
| upsertStmt.run('recovery_ciphertext', encryptedRecovery.ciphertext); | ||
| upsertStmt.run('recovery_iv', encryptedRecovery.iv); | ||
| upsertStmt.run('recovery_tag', encryptedRecovery.tag); | ||
| upsertStmt.run('admin_setup_complete', '1'); | ||
| upsertStmt.run('failed_login_attempts', '0'); | ||
| })(); |
Contributor
There was a problem hiding this comment.
Do not persist the bcrypt verifier in plaintext.
admin_password_hash is written directly to test_config here and again in the recovery/password-change paths. That leaves an offline-crackable credential verifier in the database even though the LMK path already exists for sensitive material. Encrypt the hash before storing it and decrypt only at compare/update sites.
As per coding guidelines, src/main/key-management.ts: "All sensitive data (password hash, recovery key material) must be encrypted with keys bound to the LMK before storage."
Also applies to: 395-413, 450-454
🤖 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 `@src/main/auth.ts` around lines 67 - 91, The admin_password_hash is being
stored in plaintext (passwordHash) in test_config; instead encrypt the bcrypt
verifier with the existing LMK flow before persisting and only decrypt it where
comparisons/updates occur: call encryptWithLMK(passwordHash) and store
encryptedPassword.ciphertext, .iv, .tag under dedicated keys (e.g.,
admin_password_ciphertext/admin_password_iv/admin_password_tag) instead of
admin_password_hash, update the db.transaction block that uses upsertStmt and
the functions that perform recovery/password-change to decrypt via
decryptWithLMK when comparing or re-hashing, and remove any other direct writes
of passwordHash (e.g., the other locations mentioned) so all password verifier
storage follows the LMK encryption pattern used for recoveryKey.
Comment on lines
+105
to
+166
| // Check lockout | ||
| const lockoutRow = db | ||
| .prepare('SELECT value FROM test_config WHERE key = ?') | ||
| .get('lockout_until') as { value: string } | undefined; | ||
| if (lockoutRow) { | ||
| const lockoutUntil = parseInt(lockoutRow.value, 10); | ||
| if (Date.now() < lockoutUntil) { | ||
| const remaining = Math.ceil((lockoutUntil - Date.now()) / 1000); | ||
| throw new Error(`Account locked. Please try again in ${remaining} seconds`); | ||
| } | ||
| // Lockout expired - reset it | ||
| db.prepare('UPDATE test_config SET value = ? WHERE key = ?').run('0', 'lockout_until'); | ||
| } | ||
|
|
||
| const hashRow = db | ||
| .prepare('SELECT value FROM test_config WHERE key = ?') | ||
| .get('admin_password_hash') as { value: string } | undefined; | ||
| if (!hashRow) throw new Error('Admin not registered'); | ||
|
|
||
| const isValid = await bcrypt.compare(password, hashRow.value); | ||
|
|
||
| if (isValid) { | ||
| // Reset failure counter | ||
| db.prepare('UPDATE test_config SET value = ? WHERE key = ?').run('0', 'failed_login_attempts'); | ||
| db.prepare('UPDATE test_config SET value = ? WHERE key = ?').run('0', 'lockout_until'); | ||
|
|
||
| const sessionToken = randomBytes(32).toString('hex'); | ||
| const expiryMs = Date.now() + SESSION_DURATION; | ||
| const nowNs = process.hrtime.bigint(); | ||
| const expiryNs = nowNs + BigInt(SESSION_DURATION) * 1_000_000n; | ||
| activeSessions.set(sessionToken, { webContentsId, expiry: expiryNs }); | ||
|
|
||
| if (db) { | ||
| db.prepare('INSERT OR REPLACE INTO test_config (key, value) VALUES (?, ?)').run( | ||
| 'session_expiry', | ||
| expiryMs.toString() | ||
| ); | ||
| } | ||
|
|
||
| return { sessionToken }; | ||
| } else { | ||
| // Handle failure and lockout | ||
| const attemptsRow = db | ||
| .prepare('SELECT value FROM test_config WHERE key = ?') | ||
| .get('failed_login_attempts') as { value: string } | undefined; | ||
| const attempts = parseInt(attemptsRow?.value || '0', 10) + 1; | ||
|
|
||
| db.prepare('UPDATE test_config SET value = ? WHERE key = ?').run( | ||
| attempts.toString(), | ||
| 'failed_login_attempts' | ||
| ); | ||
|
|
||
| if (attempts >= 5) { | ||
| const lockoutUntil = Date.now() + 60 * 1000; // 1 minute | ||
| db.prepare('UPDATE test_config SET value = ? WHERE key = ?').run( | ||
| lockoutUntil.toString(), | ||
| 'lockout_until' | ||
| ); | ||
| throw new Error('Too many failed attempts. Account locked for 1 minute'); | ||
| } | ||
|
|
||
| throw new Error('Invalid password'); |
Contributor
There was a problem hiding this comment.
Track lockouts by device UUID, not with global keys.
failed_login_attempts and lockout_until are stored as single global test_config entries. That means one device can trigger lockout for every other device, and a successful login on any device clears the shared counter. This needs to be keyed by the persistent device UUID instead of a single process-wide bucket.
As per coding guidelines, src/main/auth.ts: "Failed login or recovery attempts must be counted per device UUID; after 5 failures the account enters a 1-minute lockout; counter resets on success."
Comment on lines
+296
to
+326
| export async function validateRecoveryKey(recoveryKey: string): Promise<{ valid: boolean }> { | ||
| if (!db) throw new Error('Database not initialized'); | ||
|
|
||
| // Validate format: 64-character hex string | ||
| if (!/^[a-fA-F0-9]{64}$/.test(recoveryKey)) { | ||
| return { valid: false }; | ||
| } | ||
|
|
||
| // Get stored recovery key (encrypted) | ||
| const storedC = db | ||
| .prepare('SELECT value FROM test_config WHERE key = ?') | ||
| .get('recovery_ciphertext') as { value: string } | undefined; | ||
| const storedIv = db.prepare('SELECT value FROM test_config WHERE key = ?').get('recovery_iv') as | ||
| | { value: string } | ||
| | undefined; | ||
| const storedTag = db | ||
| .prepare('SELECT value FROM test_config WHERE key = ?') | ||
| .get('recovery_tag') as { value: string } | undefined; | ||
|
|
||
| if (!storedC || !storedIv || !storedTag) { | ||
| return { valid: false }; | ||
| } | ||
|
|
||
| // Decrypt stored key and compare | ||
| const decryptedStored = await decryptWithLMK(storedC.value, storedIv.value, storedTag.value); | ||
|
|
||
| if (recoveryKey.toLowerCase() === decryptedStored.toLowerCase()) { | ||
| return { valid: true }; | ||
| } | ||
|
|
||
| return { valid: false }; |
Contributor
There was a problem hiding this comment.
Split recovery into verify → reset-token → password reset.
validateRecoveryKey() only reports validity, while performRecovery() accepts the raw recovery key and new password in the same IPC path. That makes the long-lived recovery key the reset credential and skips the required short-lived reset-token step; it also leaves failed recovery attempts untracked per device.
As per coding guidelines, src/main/auth.ts: "Recovery flow must be two-step: (1) verify the recovery key to generate a password-reset token, (2) accept a new password and re-encrypt stored keys" and "Failed login or recovery attempts must be counted per device UUID; after 5 failures the account enters a 1-minute lockout; counter resets on success."
Also applies to: 343-430
🤖 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 `@src/main/auth.ts` around lines 296 - 326, validateRecoveryKey currently only
checks the provided recovery key and performRecovery consumes the raw recovery
key and new password in one step; split the flow into two steps: (1) update
validateRecoveryKey to verify the provided recoveryKey using decryptWithLMK and,
on success, generate and store a short-lived reset token (linked to the account)
instead of accepting a new password, returning only validity and the token
expiry info; (2) change performRecovery to accept the reset token plus the new
password, validate the token against the stored token, then re-encrypt stored
keys and rotate the recovery credential; additionally implement per-device
failure counting tied to device UUID (increment failures on invalid recoveryKey
or invalid reset token in validateRecoveryKey and performRecovery respectively,
reset counter on success, and enforce a lockout after 5 failures for 1 minute)
and reuse DB entries already referenced (test_config keys and functions like
decryptWithLMK) to persist tokens, counters, timestamps, and lockout state.
Comment on lines
+34
to
+64
| export async function getOrCreateLMK(): Promise<string> { | ||
| try { | ||
| // Determine keytar service and account from DB config, with defaults | ||
| let service = DEFAULT_KEYTAR_SERVICE; | ||
| let account = DEFAULT_KEYTAR_ACCOUNT; | ||
|
|
||
| if (db) { | ||
| const serviceRow = db | ||
| .prepare('SELECT value FROM test_config WHERE key = ?') | ||
| .get(DB_KEY_KEYTAR_SERVICE) as { value: string } | undefined; | ||
| if (serviceRow?.value) { | ||
| service = serviceRow.value; | ||
| } | ||
|
|
||
| const accountRow = db | ||
| .prepare('SELECT value FROM test_config WHERE key = ?') | ||
| .get(DB_KEY_KEYTAR_ACCOUNT) as { value: string } | undefined; | ||
| if (accountRow?.value) { | ||
| account = accountRow.value; | ||
| } | ||
| } | ||
|
|
||
| const existingKey = await keytar.getPassword(service, account); | ||
| if (existingKey) { | ||
| return existingKey; | ||
| } | ||
|
|
||
| // Generate 32 random bytes for AES-256 | ||
| const newKey = randomBytes(32).toString('hex'); | ||
| await keytar.setPassword(service, account, newKey); | ||
| return newKey; |
Contributor
There was a problem hiding this comment.
Make the LMK derivation installation-bound.
getOrCreateLMK() currently generates a random key and stores it in a static keytar slot, while getOrCreateDeviceUUID() never feeds into that derivation. That misses the required design where the LMK is derived from OS-native secret material and salted with the persistent device UUID, so the encryption root is not actually bound to the installation identity.
As per coding guidelines, src/main/key-management.ts: "LMK (Local Master Key) must be a device-specific key derived from the OS keychain / native crypto and serve as the root of all encryption" and "Device UUID must be a persistent identifier generated once per device installation; used as a salt component in key derivation and as the session identifier."
Also applies to: 140-159
🤖 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 `@src/main/key-management.ts` around lines 34 - 64, getOrCreateLMK currently
generates a random key and stores it directly; instead derive the LMK
deterministically from OS-native secret material plus the persistent device UUID
so the key is installation-bound. Modify getOrCreateLMK to: 1) read an OS-secret
from keytar (use the same service/account logic with
DEFAULT_KEYTAR_SERVICE/DEFAULT_KEYTAR_ACCOUNT and
DB_KEY_KEYTAR_SERVICE/DB_KEY_KEYTAR_ACCOUNT); 2) if the OS-secret is missing,
generate a high-entropy OS-secret (randomBytes) and store it via
keytar.setPassword; 3) call getOrCreateDeviceUUID() to obtain the persistent
device UUID and use it as the salt; 4) derive a 32-byte LMK using a standard KDF
(HKDF-SHA256 or HMAC-based KDF) from the OS-secret with the device UUID as salt
and return that derived key (do not replace the stored OS-secret with the
derived LMK). Update any code paths that used randomBytes(32).toString('hex') so
they use the derived output instead.
Comment on lines
+32
to
+44
| /** Handles backdrop click to close the modal. */ | ||
| const handleBackdropClick = (e: React.MouseEvent<HTMLDivElement>) => { | ||
| if (e.target === e.currentTarget && !mandatory) { | ||
| onClose?.(); | ||
| } | ||
| }; | ||
|
|
||
| /** Handles keydown events (Escape key) to close the modal. */ | ||
| const handleKeyDown = (e: KeyboardEvent<HTMLDivElement>) => { | ||
| if (e.key === 'Escape' && !mandatory) { | ||
| onClose?.(); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Reset sensitive state on every dismiss path.
Backdrop/Escape/back/success all call onClose directly, so the last password and error stay in component state and can reappear when the modal opens again. Route every exit through a shared handleClose() that clears local state first.
Also applies to: 84-88, 132-148
🤖 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 `@src/renderer/components/Admin/AdminLoginModal.tsx` around lines 32 - 44,
Create a single shared handleClose() that first clears the component's sensitive
local state (reset password and error via setPassword('') and setError(null/''
as appropriate) or whatever state setters are used) and then calls onClose?.();
replace direct calls to onClose in handleBackdropClick and handleKeyDown with
handleClose, and update the other exit paths referenced (the back button handler
and the success/submit-complete handler) to call handleClose as well so every
dismiss path clears sensitive state before closing.
Comment on lines
+37
to
+50
| // Determine which phase to show | ||
| if (recoveryKey) { | ||
| return ( | ||
| <RecoveryKeyPhase | ||
| t={t} | ||
| recoveryKey={recoveryKey} | ||
| hasSavedKey={hasSavedKey} | ||
| isLoading={isLoading} | ||
| onHasSavedKeyChange={setHasSavedKey} | ||
| onCopy={handleCopyKey} | ||
| onContinue={handleContinue} | ||
| onClose={handleClose} | ||
| /> | ||
| ); |
Contributor
There was a problem hiding this comment.
Keep the recovery-key step inside the modal shell.
This branch returns the recovery-key card without the fixed backdrop or dialog semantics, so the page behind it stays interactive and the second step is no longer a real modal. Wrap RecoveryKeyPhase in the same overlay/role="dialog" container as the registration step.
Also applies to: 279-355
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 46-46: src/renderer/components/Admin/AdminRegisterModal.tsx#L46
Promise-returning function provided to attribute where a void return was expected.
[warning] 47-47: src/renderer/components/Admin/AdminRegisterModal.tsx#L47
Promise-returning function provided to attribute where a void return was expected.
🤖 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 `@src/renderer/components/Admin/AdminRegisterModal.tsx` around lines 37 - 50,
The RecoveryKeyPhase is being returned directly which bypasses the modal shell
and dialog semantics; instead wrap the RecoveryKeyPhase component in the same
overlay/modal container used by the registration step (the modal backdrop and
element with role="dialog" in AdminRegisterModal) so it renders inside the fixed
modal shell and retains focus/aria semantics; update the branch that returns
RecoveryKeyPhase (and the other occurrence later in the file that mirrors lines
~279-355) to render the overlay/backdrop and the dialog container and then
render <RecoveryKeyPhase .../> inside it, preserving the same props and event
handlers.
Comment on lines
+12
to
+16
| /** | ||
| * Modal wrapper for password recovery using the direct recovery key path only. | ||
| * Email recovery is temporarily disabled. | ||
| */ | ||
| export const RecoveryModal: FC<RecoveryModalProps> = ({ isOpen, onClose }) => { |
Contributor
There was a problem hiding this comment.
This no longer matches the required recovery contract.
The component only supports validateRecoveryKey → performRecovery, but the required RecoveryModal flow is requestRecovery → reset token → completeRecovery. That leaves the specified email/reset-token recovery path unavailable from this UI.
As per coding guidelines: RecoveryModal must use requestRecovery in step 1 and completeRecovery in step 2.
Also applies to: 87-114
🤖 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 `@src/renderer/components/Admin/RecoveryModal.tsx` around lines 12 - 16, The
RecoveryModal currently implements the direct key flow using validateRecoveryKey
and performRecovery; change it to implement the required email/reset-token flow
by replacing calls to validateRecoveryKey and performRecovery with
requestRecovery for step 1 (trigger email/reset-token) and completeRecovery for
step 2 (submit reset token and new password). Update the component logic
(RecoveryModal) to accept and handle the reset-token input/state, call
requestRecovery when the user submits their email, render the token/password
form after requestRecovery succeeds, and call completeRecovery to finish
recovery; ensure error/success paths and prop/signature usages are adjusted
accordingly where validateRecoveryKey/performRecovery are currently referenced.
Comment on lines
+35
to
+41
| return ( | ||
| <div | ||
| className="fixed inset-0 z-50 flex items-center justify-center bg-black/50" | ||
| role="dialog" | ||
| aria-modal="true" | ||
| aria-labelledby="recovery-title" | ||
| > |
Contributor
There was a problem hiding this comment.
Backdrop dismissal is wired to the content panel.
The outer overlay has no close handlers, while the inner white card does. Result: clicks on the dark backdrop do nothing, but clicking empty padding inside the card can close the modal. Move the click/Escape handlers to the overlay and keep the panel passive.
Also applies to: 141-160
🤖 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 `@src/renderer/components/Admin/RecoveryModal.tsx` around lines 35 - 41, The
backdrop dismissal handlers are attached to the inner panel instead of the
overlay; move the click and Escape close logic from the white card element to
the outer overlay div so clicks on the dark backdrop and pressing Escape close
the modal while the panel remains passive. Specifically, remove any
onClick/onKeyDown (or onKeyUp) close handlers from the panel element in
RecoveryModal and attach a click handler on the overlay div that calls the
existing close function (and add a useEffect or overlay-level keydown listener
to handle Escape if currently handled on the panel). Repeat the same change for
the duplicate modal block referenced around the 141-160 region so both overlay
divs manage dismissal and the inner panels have no close handlers.
Comment on lines
39
to
41
| /** | ||
| * Handles modal close button click | ||
| */ |
Contributor
There was a problem hiding this comment.
Comment is misleading.
The handleCancel function handles the native close event (e.g., Escape key), not button clicks. The close button on line 109 calls onClose directly.
📝 Proposed fix for the comment
/**
- * Handles modal close button click
+ * Handles native dialog close event (e.g., Escape key)
*/
const handleCancel = () => {🤖 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 `@src/renderer/components/Results/AcsCalculationModal/AcsCalculationModal.tsx`
around lines 39 - 41, Update the misleading JSDoc above handleCancel: change the
text to state that handleCancel handles the native modal close event (e.g.,
Escape key or dialog close), not the modal's close button, and note that the
close button calls onClose directly; edit the comment near the handleCancel
function to reflect this distinction (referencing handleCancel and onClose).
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.
Summary by CodeRabbit
New Features
Documentation
Chores