Conversation
|
Here's the code health analysis summary for commits Analysis Summary
|
Co-authored-by: AartSchinkel <aartschinkel@iservecloud.com> Co-authored-by: Meier Lukas <meierschlumpf@gmail.com>
28c7159 to
adc40bb
Compare
|
According to cursor ai, you could refactor |
|
Cursor Ai also recommended to change Not sure how or why this would work but might be worth trying since the code is here and it sounds like of the devs have a bench marking mechanism setup (maybe its some nice low hanging fruit) |
|
How can i get access to submitting a PR with the suggestions Cursor has made along with some other ones its told me about? It might be easier for me to raise a PR for you guys to test? @manuel-rw @Meierschlumpf |
|
You can fork, check out this branch, make your changes and then push to your fork. Then, create a PR targeting this branch. Note that AI suggestions can be hallucinated, please review your changes yourself and test whether they work. |
How are you guys checking memory usage for the application?
How do i run this project locally? Ive forked, cloned and:
|
|
Hi, you would probably be better off forking from |
| COPY . . | ||
|
|
||
| RUN corepack enable pnpm && pnpm install --recursive --frozen-lockfile | ||
| RUN corepack enable pnpm && pnpm install --recursive --no-frozen-lockfile |
| ARG CI='true' | ||
| ARG DISABLE_REDIS_LOGS='true' | ||
|
|
||
| # Create database directory for build-time database access |
There was a problem hiding this comment.
Comment is wrong, why touch an empty file? Remove?
| COPY --from=builder /app/apps/tasks/tasks.cjs ./apps/tasks/tasks.cjs | ||
| COPY --from=builder /app/apps/websocket/wssServer.cjs ./apps/websocket/wssServer.cjs | ||
| # Tasks worker and WebSocket are now merged into Next.js server, so no separate builds needed | ||
| # COPY --from=builder /app/apps/tasks/tasks.cjs ./apps/tasks/tasks.cjs |
| experimental: { | ||
| optimizePackageImports: ["@mantine/core", "@mantine/hooks", "@tabler/icons-react"], | ||
| turbopackFileSystemCacheForDev: true, | ||
| // Reduce memory usage by limiting concurrent requests |
There was a problem hiding this comment.
Comments are not very useful, remove
| }, | ||
| ], | ||
| minimumCacheTTL: 60, | ||
| dangerouslyAllowSVG: true, |
|
|
||
| function getBaseUrl() { | ||
| return `http://localhost:${CRON_JOB_API_PORT}`; | ||
| // Tasks API is now merged into Next.js, so use the same port |
There was a problem hiding this comment.
Not useful comment, remove
| # Node.js memory optimization flags: | ||
| # --optimize-for-size: Optimizes for smaller memory footprint (trades some performance for memory) | ||
| # --max-old-space-size: Limit heap size to prevent memory bloat (set to reasonable limit) | ||
| # --expose-gc: Expose garbage collection API (allows manual GC if needed) |
There was a problem hiding this comment.
Is it a good Idea to expost this in production?
| @@ -0,0 +1,347 @@ | |||
| # Migration Summary: libsql-js Migration & Performance Optimizations | |||
There was a problem hiding this comment.
Remove this file entirely and move documentation if necessary to other places. It doesn't add any value and is just a changelog.
| @@ -0,0 +1,69 @@ | |||
| .PHONY: help build start stop restart rebuild logs logs-homarr logs-redis status health shell clean clean-all | |||
There was a problem hiding this comment.
Why do we need such scripts?
They have nothing to do with the actual memory change.
And they are in platform agnostic languages. Why not use an independent one, such as Python for this?
| @@ -0,0 +1,40 @@ | |||
| version: '3.8' | |||
There was a problem hiding this comment.
File not needed, delete
| expires: expires.getTime(), // Convert Date to milliseconds timestamp | ||
| userId: user.id, | ||
| }); | ||
| logger.info(`Session created successfully for user ${user.id}`); |
There was a problem hiding this comment.
Remove or reduce log level
| // The schema defines expires as int({ mode: "timestamp_ms" }) | ||
| await adapter.createSession({ | ||
| sessionToken, | ||
| expires: expires.getTime(), // Convert Date to milliseconds timestamp |
There was a problem hiding this comment.
Doesn't compile, vibe coded?
| if (options.password) { | ||
| console.log(`Password for user ${options.username} has been set to the provided value`); | ||
| } else { | ||
| console.log(`New password for user ${options.username}: ${newPassword}`); | ||
| } |
|
@Meierschlumpf I think we should split this into the following changes / actions:
I believe we should do these changes incrementally and step by step;
What do you think? |
|
With libsql I couldnt get authentication to work so you will see in my version that the better-sqlite is only used for authentication which I was still working on. Further I threw all code through Cursor to bug check and make comments on it sothat you could what was changed, thats were the workspace folder was made. Just check that libsql supports authentication on the webpage or otherwise we need to rewrite that piece to make it work with libsql instead of better-sqlite. |
Homarr
Thank you for your contribution. Please ensure that your pull request meets the following pull request:
pnpm build, autofix withpnpm format:fix)devbranchx,y,ior any abbrevation)TODO:
devpotential fix for #3759 , code by @AartSchinkel