Make Github stars dynamic and improve database init #5000
Conversation
| @@ -0,0 +1,390 @@ | |||
| { | |||
There was a problem hiding this comment.
I think we don't want to commit that? Goal would be to get rid of sqlite
| @@ -0,0 +1,4 @@ | |||
| CREATE TABLE `githubStars` ( | |||
| `id` text PRIMARY KEY NOT NULL, | |||
| `numberOfStars` integer | |||
There was a problem hiding this comment.
It feels odd to have this structure.
I would rather (1) create a more generic key/value table (e.g githubInfo with last release number, total star count, etc.).
Or (2) make this a time series table by introducing a date field (and then you only retrieve the most recent one).
|
|
||
| console.log('Synching data..'); | ||
|
|
||
| const query = graphql.defaults({ |
There was a problem hiding this comment.
This code is related to Github, shouldn't be in the database folder. What you should do instead is isolate the logic into a folder dedicated to the Github sync. Also make sure you don't copy/paste too much code, this seems very similar to existing code in init/route.tsx?
| export const AppHeader = async () => { | ||
| const githubStars = await findOne( | ||
| githubStarsModel, | ||
| desc(githubStarsModel.id), |
There was a problem hiding this comment.
You've set a text primary key (uuid?). I guess desc() would make sense if you had an auto-incrementing integer key but here it doesn't really?
| @@ -25,7 +37,9 @@ export const HeaderDesktop = () => { | |||
| Docs <ExternalArrow /> | |||
| </ListItem> | |||
| <ListItem href="https://github.com/twentyhq/twenty"> | |||
| <GithubIcon color="rgb(71,71,71)" /> 8.3k <ExternalArrow /> | |||
| <GithubIcon color="rgb(71,71,71)" />{' '} | |||
There was a problem hiding this comment.
Do we need {' '} ? Does this reflect a gap/margin issue?
|
|
||
| export const HeaderDesktop = ({ numberOfStars }: Props) => { | ||
| const formatNumberOfStars = (numberOfStars: number) => { | ||
| if (numberOfStars < 1000) { |
There was a problem hiding this comment.
Hopefully we should never need this 😁.
Makes sens if you put that function in a util (which would be nice, could be not just for stars) but since you kept it in that context it should be as short and focused on that use-case as possible
| return ( | ||
| <> | ||
| <HeaderDesktop /> | ||
| <HeaderDesktop numberOfStars={githubStars?.[0]?.numberOfStars} /> |
There was a problem hiding this comment.
Can you expose the formatted number of stars as an endpoint for Thomas to retrieve on Framer?
|
Applied the required modifications |
packages/twenty-website/package.json
Outdated
| @@ -8,7 +8,7 @@ | |||
| "build": "next build", | |||
| "start": "next start", | |||
| "lint": "next lint", | |||
| "database:init": "npx tsx src/database/init-database.ts", | |||
| "github:sync": "npx tsx src/database/init-database.ts", | |||
There was a problem hiding this comment.
is it a github sync or a database init? you kept the file name init database
| @@ -1,6 +1,6 @@ | |||
| import { graphql } from '@octokit/graphql'; | |||
|
|
|||
| import { Repository } from '@/app/contributors/api/types'; | |||
| import { Repository } from '@/github-synch/contributors/types'; | |||
There was a problem hiding this comment.
synch is an unusual name, we usually use sync or synchronization (sync is better imo)
| return ( | ||
| <> | ||
| <HeaderDesktop /> | ||
| <HeaderDesktop numberOfStars={githubStars?.[0]?.numberOfStars} /> | ||
| <HeaderMobile /> |
There was a problem hiding this comment.
I think we also show the number of stars on mobile?
|
Implemented the changes |
|
It's working on my side, I couldn't reproduce the error. Let me know if it's working for you ! |
|
I'm not sure it will work when rolling out it production. It didn't work for me as an upgrade to the previous migration (it does work if you start from scratch of course though). Let's merge and see! |
I extracted the init database logic into its own file. You can now run it with yarn database:init. Added database entry for GitHub stars. Do you want me to remove the init route or is it used for something else ? --------- Co-authored-by: Ady Beraud <[email protected]> Co-authored-by: Félix Malfait <[email protected]>
I extracted the init database logic into its own file.
You can now run it with yarn database:init.
Added database entry for GitHub stars.
Do you want me to remove the init route or is it used for something else ?