-
Notifications
You must be signed in to change notification settings - Fork 8
copilot #27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
copilot #27
Conversation
同时修改了/develop/docker-compose.dev.yml中references和notifications端口冲突的问题 |
develop/docker-compose.dev.yml
Outdated
- NODE_OPTIONS=--inspect=0.0.0.0:9229 | ||
ports: | ||
- "127.0.0.1:9230:9229" | ||
- "0.0.0.0:9230:9229" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
为什么要这么改呢?这会有一定的安全问题
develop/bin/dev
Outdated
#!/usr/bin/env bash | ||
|
||
docker-compose -f docker-compose.yml -f docker-compose.dev.yml up --no-deps --detach "$@" | ||
docker compose -f docker-compose.yml -f docker-compose.dev.yml up --no-deps --detach "$@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
是不是可以把针对开发环境 (dev) 的修改新开一个 PR?(下面还有很多也是一样的)
develop/compiles/.gitkeep
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
像是误删?
develop/docker-compose.dev.yml
Outdated
- ../services/llm/app.js:/overleaf/services/llm/app.js | ||
- ../services/llm/config:/overleaf/services/llm/config | ||
- ../services/llm/controllers:/overleaf/services/llm/controllers | ||
- ../services/llm/models:/overleaf/services/llm/models | ||
- ../services/llm/routes:/overleaf/services/llm/routes | ||
- ../services/llm/services:/overleaf/services/llm/services | ||
- ../services/llm/utils:/overleaf/services/llm/utils | ||
- ../services/llm/package.json:/overleaf/services/llm/package.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
文件目录结构是不是可以改得和其他模块一样?其他模块都是只需要挂载 app/
, app.js
, config/
,这里分太多目录是不是没有必要
services/llm/.vscode/settings.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VS Code 的 config 不应该出现
libraries/settings/index.js
Outdated
@@ -1 +1 @@ | |||
module.exports = require('./Settings') | |||
module.exports = require('./Settings') No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
可能是编辑器配置的问题?好多文件都是最后一行没有换行 (no newline at the end of file)?
services/llm/config/db.js
Outdated
export default async function connectDatabase() { | ||
try { | ||
await mongoose.connect(dbConfig.uri, dbConfig.options); // 使用 mongoose.connect | ||
console.log('MongoDB 连接成功'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
能不能尽量用英文啊?因为原来的 Overleaf 项目都是用英文的
services/llm/config/db.js
Outdated
import mongoose from 'mongoose'; // 新增 | ||
import {MONGO_URL} from './settings.defaults.js'; | ||
|
||
// 数据库配置 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
有很多类似的无用 comment,像是 LLM 给出来的。大家都知道 dbConfig
是“数据库配置”的意思。建议清理一下(最好是能用英文注释)
services/llm/config/db.js
Outdated
@@ -0,0 +1,23 @@ | |||
import mongoose from 'mongoose'; // 新增 | |||
import {MONGO_URL} from './settings.defaults.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
你看其他 service 模块从来没有直接从 settings.defaults.js
里面导入的,都是用的
const Settings = require('@overleaf/settings')
这里面有一些特殊的逻辑在里面的
….dev.ymlip限制4.排除不必要文件5.解决newline at the end of file等
….dev.ymlip限制4.排除不必要文件5.解决newline at the end of file等
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The recommended development environment is to use docker as defined in /develop/README.md . We can keep this .gitignore as is.
develop/docker-compose.dev.yml
Outdated
- ../services/llm/app.js:/overleaf/services/llm/app.js | ||
- ../services/llm/config:/overleaf/services/llm/config | ||
- ../services/llm/app:/overleaf/app | ||
- ../services/llm/package.json:/overleaf/services/llm/package.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unnecessary because when this image is built as defined in /develop/docker-compose.yml , node_modules are already installed. No need to reimport package.json here. Also, follow the order of other services is recommended, that
- ../services/notifications/app:/overleaf/services/notifications/app
- ../services/notifications/app.js:/overleaf/services/notifications/app.js
- ../services/notifications/config:/overleaf/services/notifications/config
- llm | ||
|
||
webpack: | ||
user: root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary for this service to run?
package.json
Outdated
"services/tpdsworker", | ||
"services/web" | ||
"services/web", | ||
"services/llm" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider sort dependencies in alphabetical order to comply with the conventions.
services/llm/Dockerfile
Outdated
@@ -0,0 +1,22 @@ | |||
FROM node:18.20.2 AS base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider bump this version to latest lts as this service is recently written
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
English or Chinese prompt?
services/llm/docker-compose.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other services, docker-compose.yml serves as unit test. This doesn't seem useful if there is no unit test but only depenedencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update these comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frontend should be i18n compatible, or at least in English since currently deployed instance is set to use English.
添加了overleaf copilot功能