feat: add remember password function#1317
Conversation
|
@Pakchoioioi @Wendong-Fan Could you please review the PR? |
|
thanks @bittoby ! could @4pmtong and @fengju0213 help reviewing this feature? |
|
@Wendong-Fan @4pmtong @fengju0213 I'd really appreciate your feedback! |
nitpicker55555
left a comment
There was a problem hiding this comment.
This PR has several serious issues. A pure function is wrapped in useMemo even though it does not depend on any state and should be defined outside the component. Returning a function from useMemo with an empty dependency array is meaningless and looks like AI-driven over-optimization. The implementation stores plaintext passwords instead of persisting the existing auth token via setAuth, which indicates the current authentication flow was not properly reviewed. There is also a redundant map in credentials-load where the input and output shapes are identical, making it unnecessary code. In addition, credentialsLoad() has no error handling and lacks a catch on the promise.
Considering the extent of these problems, iterating through review cycles would likely cost more effort than rewriting it. I suggest closing this PR and submitting a clean one instead. @Wendong-Fan @4pmtong
|
@nitpicker55555 please don't close this PR. I will update in this PR |
|
@nitpicker55555 I updated to follow your feedback. What i have changed:
|
thanks @bittoby will review it asap |
fengju0213
left a comment
There was a problem hiding this comment.
thanks @bittoby overall its good,I'm wondering if automatic autofill is sufficient, and automatic login isn't necessary, because there might be other operations on this login page later, such as changing the password, etc.
cc @Pakchoioioi
2ca5bc1 to
4336b3b
Compare
|
@fengju0213 It passed all tests |
|
@Wendong-Fan @bytecii Any update for me, please |
Sorry for the late reply. Since this involves user experience, I will ask our product team to provide feedback today. |
|
Thank you @fengju0213 |
|
Would someone please merge this PR? |
|
Hi @bittoby Thanks for contribution, just let product team test. Right now the redirect feels a bit too sudden, but it can be improved with slight modifications:
|
f7f6104 to
0306741
Compare
|
@Pakchoioioi I updated logic based on your feedbacks. thanks for your feedback. And Testing fails doesn't seem to relate to my changes. Please check. test.webm |
|
Thanks @bittoby . Could @Pakchoioioi @Douglasymlai help review whether the current approach makes sense? |
thanks @bittoby I think the "remember me" status should be persisted? |
|
@fengju0213 Good point! I solved that. |
|
@Wendong-Fan @Pakchoioioi please any updates for me... |
|
@Wendong-Fan @Pakchoioioi Please review this PR. I opened this PR last 3 weeks. I think it is ready to merge now. |
|
@nitpicker55555 i think pr can be merged now |
f792313 to
53f1f33
Compare
|
Hi @bittoby We truly appreciate your commitment and the valuable work you’ve contributed to the login in feature I would like to explain the situation regarding this feature. As our product client offers both download and local deployment versions, our design philosophy is to centralize all login, management, and privacy settings on the Eigent website. This allows the client to remain fully simplified while keeping the architecture consistent with mainstream coworking products. We have already implemented support for this login method in the main branch as well as in version 0.0.88. Unfortunately, this means that merging your proposed feature as-is would conflict with our current product design. To ensure your work is not lost, we will keep your branch as a reference for future development. Once again, thank you for your understanding, patience, and generous support of the project. |
|
So you mean, you can't merge my pr in the future? |
|
@Wendong-Fan @Pakchoioioi I have spent many time and effort for this pr last one month. If possible, would appreciate to merge this pr. I can update to avoid conflicts with your current product design. please help me 🙏 |
|
Sorry about that, and I really appreciate the effort you’ve put into this PR. @bittoby I won’t be able to merge it into the main branch, as it’s no longer aligned with the current login and signup logic. We also won’t have a login page in the client for now. That said, your work is still valuable, so the best I can do is keep this branch as a reference for potential future iterations if the direction evolves. Maintaining consistency and simplicity in the core system is important for the long-term health of the project. Thanks for your understanding. |
|
Ok, @Pakchoioioi, I understand and agree your decision. |
Revert remember-me credential storage and login form changes to match main. Keep a single fix: include setModelType, setInitState, and setIsFirstLaunch in the ProtectedRoute useEffect dependency list (same as main, one line). Made-with: Cursor
Made-with: Cursor



Related Issue
Closes #1316
Description
Adds secure "Remember Me" credential storage for the login page using Electron's built-in
safeStorageAPI, which encrypts credentials via OS-level keychain (macOS Keychain, Windows DPAPI, Linux Secret Service). No new dependencies required.What it does:
~/.eigent/saved_credentials.enc(file permissions0o600)Files changed:
electron/main/index.ts— 3 IPC handlers (credentials-save,credentials-load,credentials-remove) usingsafeStorage.encryptString()/decryptString()electron/preload/index.ts— Exposed credential APIs to renderer viaelectronAPIbridgesrc/types/electron.d.ts— TypeScript declarations for new credential methodssrc/pages/Login.tsx— Remember Me checkbox, saved accounts dropdown with outside-click dismiss, auto-fill on mountremember-me,saved-accounts,remove-accountkeys (en, zh-Hans, zh-Hant, ja, ko, de, fr, es, it, ru, ar)Testing Evidence (REQUIRED)
login.mp4
What is the purpose of this pull request?
Contribution Guidelines Acknowledgement