chore: Move GitHub logic out of the frontend #6307
Conversation
There was a problem hiding this comment.
This is a big PR, but the backend looks good to me. I had a question about those redundant values saved in Settings. IMHO only github_token is needed, and we can compute the rest. We can compute them in the BE if you want, so that nothing changes for FE, we just don't save them.
If you feel strongly that they need saved, okay, but please note that then it's a bit more difficult to remove them later, because they will be on users' machines.
|
Haven't confirmed that this is specific to your branch, but if I try and start a session with no settings, I just get a 400 error instead of the settings modal opening. Ideally if we load the page and there are no settings, the modal just opens automatically 
Similarly, trying to connect a GitHub account without settings set causes a hard-to-diagnose error. FE says 401, backend logs say: |
|
Just tried latest and got a wild rush of toasts |
| @@ -21,6 +21,7 @@ class Settings(BaseModel): | |||
| llm_api_key: SecretStr | None = None | |||
| llm_base_url: str | None = None | |||
| remote_runtime_resource_factor: int | None = None | |||
| github_token: str | None = None | |||
There was a problem hiding this comment.
Maybe good for a follow-up, rather than add to this large PR, but can we use SecretStr inline with llm_api_key and other secrets, rather than str for github_token?
There was a problem hiding this comment.
yeah that seems like a good idea. I hate to extend this PR further but maybe it's worth doing now. Up to you @amanape
| """ | ||
|
|
||
| github_token_is_set: bool | None = None | ||
| unset_github_token: bool | None = None |
There was a problem hiding this comment.
Thank you, I like that we have a solution for this trick!
| content={'message': 'Settings stored'}, | ||
| ) | ||
| await settings_store.store(settings) | ||
| return response | ||
| except Exception as e: | ||
| logger.warning(f'Invalid token: {e}') | ||
| return JSONResponse( | ||
| status_code=status.HTTP_401_UNAUTHORIZED, | ||
| content={'error': 'Invalid token'}, |
There was a problem hiding this comment.
Nit: this catches Exception, and it can be for other reasons than the token. In fact, it seems that at line 65 we already failed because of the token, if that was the cause. Maybe we can send some other response here, because it might be confusing
This PR replaces and simplifies #6286
Give a summary of what the PR does, explaining any non-trivial design decisions
Frontend
github_tokenin the settings file store on the servergithub_tokenon the frontendgithubTokenIsSetinsteadBackend
GitHubTokenMiddlewareto retrieve and set thegithub_tokenfrom the settings file store to the request state objectrequest.state.github_tokeninstead of"X-GitHub-Token"header/user) whenPOST /settingsis called with agithub_tokenCommon
github_token,unset_github_token, andgithub_token_is_setpropertiesUI
To run this PR locally, use the following command: