-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Clean up _regroup #7970
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
Clean up _regroup #7970
Conversation
They are already integrated in apps/server/spec/etapi.
Summary of ChangesHello @eliandoran, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a comprehensive cleanup and reorganization of the project's codebase. It primarily involves consolidating and relocating various configuration and utility scripts from a temporary Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
|
📚 Documentation preview is ready! 🔗 Preview URL: https://pr-7970.trilium-docs.pages.dev ✅ All checks passed This preview will be updated automatically with new commits. |
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.
Code Review
This pull request is a significant cleanup and refactoring effort. It successfully moves numerous files from the temporary _regroup directory into more logical and permanent locations within the project structure. A major improvement is the centralization of the ESLint configuration, which enhances maintainability. The changes also include bug fixes and improvements to scripts, such as the schema export and document generation scripts. Overall, this is a very positive change for the project's health and organization. I have a couple of minor suggestions to improve the new ESLint configurations for consistency and robustness.
| const playwrightConfig = { | ||
| files: [ | ||
| "apps/server-e2e/src/**/*.spec.ts", | ||
| "apps/desktop/e2e/**/*.spec.ts" | ||
| ], | ||
| plugins: { playwright }, | ||
| // Override or add rules here | ||
| rules: { ...playwright.configs["flat/recommended"].rules, }, | ||
| languageOptions: { parser: tsParser }, | ||
| }; |
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.
For better maintainability and to align with the recommended usage of eslint-plugin-playwright, it's better to spread the flat/recommended configuration object directly and then override specific properties. This approach is cleaner and more robust against future updates to the recommended configuration.
const playwrightConfig = {
...playwright.configs["flat/recommended"],
files: [
"apps/server-e2e/src/**/*.spec.ts",
"apps/desktop/e2e/**/*.spec.ts"
],
// The recommended config does not set a parser, so we need to set it.
languageOptions: { parser: tsParser },
};| ".cache", | ||
| "tmp", | ||
| "**/dist", | ||
| "**/out-tsc", | ||
| "apps/edit-docs/demo/*", | ||
| "docs/*", | ||
| "apps/web-clipper/lib/*" |
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.
For consistency with the main ESLint configuration (eslint.config.mjs) and the previous configuration file, it would be good to also ignore the packages/* directory here. This ensures that both linting and formatting configurations have the same scope.
".cache",
"tmp",
"**/dist",
"**/out-tsc",
"apps/edit-docs/demo/*",
"docs/*",
"apps/web-clipper/lib/*",
// TODO: check if we want to format packages here as well - for now skipping it
"packages/*"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.
Code Review
This pull request is a significant cleanup and refactoring effort, moving many files into a more logical structure under apps/ and scripts/, and centralizing the ESLint configuration. The changes greatly improve the project's organization and maintainability.
My review focuses on a few areas for further improvement:
- In the
generate_document.tsscript, the database initialization could be cleaner to avoid side effects, and an import could be made more readable. - The new root ESLint configuration could be improved by scoping framework-specific rules to avoid applying them to test files.
Overall, this is a great step forward for the project's structure.
| import preact from "eslint-config-preact"; | ||
|
|
||
| const mainConfig = [ | ||
| ...preact, |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
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.
Pull request overview
This PR performs cleanup and reorganization of the codebase by moving scripts and configuration files into a more structured layout. The main changes include consolidating scripts into a top-level scripts/ directory, updating ESLint configurations to use a monorepo-friendly setup, and removing deprecated/moved files from the _regroup/ directory.
Key changes:
- Reorganizes utility scripts (translation, repo migration, icon generation) into a centralized
scripts/directory - Modernizes ESLint configuration with separate formatting and linting configs at the root level
- Removes per-app ESLint configs in favor of a single root configuration
- Updates test infrastructure with proper timeouts and new E2E tests
- Fixes the
generate_document.tsscript to properly initialize the database
Reviewed changes
Copilot reviewed 36 out of 58 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
scripts/translation/import-translations-from-weblate-csv.ts |
New script for importing translations from Weblate CSV format |
scripts/translation/create-po-file.sh |
Shell script for creating/managing PO translation files |
scripts/repo-migration/* |
Scripts used for migrating issues, discussions, and releases between repos |
scripts/icons/tray/* |
SVG icons and build script for generating tray icons at multiple DPI scales |
scripts/icons/create-icons.sh |
Master script for generating application icons for all platforms |
eslint.config.mjs |
New root ESLint configuration using flat config format with TypeScript and import sorting |
eslint.format.config.mjs |
Separate formatting-only ESLint config for code style enforcement |
package.json |
Adds new dev scripts for formatting and linting, updates dependencies |
pnpm-lock.yaml |
Dependency updates including adding lorem-ipsum, eslint-config-preact, removing eslint-plugin-react-hooks |
apps/server/scripts/generate_document.ts |
Fixed database initialization logic and corrected import statements |
apps/server/package.json |
Adds coverage script and generate-document command |
apps/client/package.json |
Adds coverage script, removes unused @eslint/js dependency |
apps/server/src/services/import/*.spec.ts |
Increases test timeout to 60 seconds for import tests |
apps/server-e2e/src/*.spec.ts |
New E2E tests for tree rendering, settings display, and note duplication |
apps/*/eslint.config.mjs |
Removes individual app ESLint configs (consolidated to root) |
_regroup/* |
Removes old/moved files including integration tests, config files, and scripts |
.gitignore |
Adds coverage directories to ignore list |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
apps/server/scripts/generate_document.ts:39
- Both
sqlInit.initializeDb()at line 38 andawait sqlInit.dbReadyat line 39 are called, but there's already a check forisDbInitialized()at line 31 that exits if the database is not initialized. This seems contradictory - if the database isn't initialized, the script exits, but then you're callinginitializeDb(). Consider clarifying the initialization logic or removing the redundant call.
| * Issues, using the "Transfer" function for each issue (via `gh cli`). | ||
| * The migration logs are available in `migrated-issues.txt`. | ||
| * Discussions, which transferred each discussion from the original repo. This one is a bit more complicated (and potentially flaky) since it works via playwright, by manually selecting the "Transfer" function (no API available at the time). | ||
| * The migration logs are available in`migrated-discussions.txt`. |
Copilot
AI
Dec 6, 2025
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.
Missing space after "in" - should be "in migrated-discussions.txt" with a space before the backtick.
No description provided.