-
Notifications
You must be signed in to change notification settings - Fork 5k
Sync built files #17379
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?
Sync built files #17379
Conversation
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 issues found across 74 files
Greptile SummaryThis PR implements functionality to upload built application files to local storage during the build process. The implementation includes a new GraphQL resolver Major changes:
Critical issue found:
Test artifacts:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as CLI (app-build)
participant FW as FunctionsWatcher
participant FCW as FrontComponentsWatcher
participant API as ApiService
participant Server as GraphQL Server
participant Storage as FileStorage
CLI->>FW: buildFunctions()
CLI->>FCW: buildFrontComponents()
FW->>FW: Build function files (.mjs)
FW->>CLI: onFileBuilt(builtPath, checksum)
CLI->>API: uploadFile(filePath, FileFolder.BuiltFunction)
API->>Server: GraphQL uploadApplicationFile mutation
Server->>Storage: Write file to storage
Storage-->>Server: File saved
Server-->>API: FileDTO response
API-->>CLI: Upload success
Note over FCW: Missing upload logic!
FCW->>FCW: Build front component files (.mjs)
FCW->>CLI: onFileBuilt(builtPath, checksum)
Note over CLI,Storage: Front components NOT uploaded
|
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.
Additional Comments (2)
-
packages/twenty-sdk/src/cli/commands/app/app-build.ts, line 107-130 (link)logic: Front components are built but NOT uploaded to storage. Only functions have upload logic in
onFileBuiltcallback (lines 85-99). Add similar upload logic for front components: -
packages/twenty-apps/toto/src/app/application.config.ts, line 1-10 (link)style:
totoandtoto1test app directories should not be committed. These appear to be test/development apps that should be in.gitignore.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
65 files reviewed, 2 comments
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:8047 This environment will automatically shut down when the PR is closed or after 5 hours. |
📊 API Changes ReportGraphQL Schema ChangesGraphQL Schema Changes[log] [log] ✖ Argument builtHandlerPath: String! added to field Mutation.uploadApplicationFile GraphQL Metadata Schema ChangesGraphQL Metadata Schema Changes[log] [log] ✖ Argument builtHandlerPath: String! added to field Mutation.uploadApplicationFile
|
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.
1 issue found across 24 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-sdk/src/cli/commands/app/app-sync.ts">
<violation number="1" location="packages/twenty-sdk/src/cli/commands/app/app-sync.ts:39">
P2: Handle upload failures before proceeding to sync so the manifest isn’t published when built files failed to upload.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
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.
4 issues found across 26 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-sdk/src/cli/__tests__/apps/root-app/__integration__/app-dev/app-dev.integration.spec.ts">
<violation number="1" location="packages/twenty-sdk/src/cli/__tests__/apps/root-app/__integration__/app-dev/app-dev.integration.spec.ts:4">
P2: The root-app spec now points to rich-app front-component tests, which assert a different output layout (src/components/*). This will fail against root-app’s root-level output and validates the wrong behavior.</violation>
<violation number="2" location="packages/twenty-sdk/src/cli/__tests__/apps/root-app/__integration__/app-dev/app-dev.integration.spec.ts:5">
P2: The root-app spec imports rich-app function tests, which expect a different output structure (nested src/functions). This mismatches root-app’s root-level function outputs.</violation>
<violation number="3" location="packages/twenty-sdk/src/cli/__tests__/apps/root-app/__integration__/app-dev/app-dev.integration.spec.ts:6">
P2: The root-app integration spec now imports rich-app manifest tests, which assert a different application name and counts. This will validate the wrong manifest for root-app and likely fail.</violation>
<violation number="4" location="packages/twenty-sdk/src/cli/__tests__/apps/root-app/__integration__/app-dev/app-dev.integration.spec.ts:8">
P2: The root-app spec now uses rich-app console output assertions, which check for different app names and counts. This will fail and validates the wrong console output for root-app.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| import { defineFunctionsTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/functions.tests'; | ||
| import { defineManifestTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/manifest.tests'; | ||
| import { runAppDev } from '@/cli/__tests__/integration/utils/run-app-dev.util'; | ||
| import { defineConsoleOutputTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/console-output.tests'; |
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.
P2: The root-app spec now uses rich-app console output assertions, which check for different app names and counts. This will fail and validates the wrong console output for root-app.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-sdk/src/cli/__tests__/apps/root-app/__integration__/app-dev/app-dev.integration.spec.ts, line 8:
<comment>The root-app spec now uses rich-app console output assertions, which check for different app names and counts. This will fail and validates the wrong console output for root-app.</comment>
<file context>
@@ -1,11 +1,11 @@
+import { defineFunctionsTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/functions.tests';
+import { defineManifestTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/manifest.tests';
+import { runAppDev } from '@/cli/__tests__/integration/utils/run-app-dev.util';
+import { defineConsoleOutputTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/console-output.tests';
const APP_PATH = join(__dirname, '../..');
</file context>
| import { defineConsoleOutputTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/console-output.tests'; | |
| import { defineConsoleOutputTests } from '@/cli/__tests__/apps/root-app/__integration__/app-dev/tests/console-output.tests'; |
| import { defineManifestTests } from './tests/manifest.tests'; | ||
| import { type RunCliCommandResult } from '@/cli/__tests__/integration/utils/run-cli-command.util'; | ||
| import { defineFrontComponentsTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/front-components.tests'; | ||
| import { defineFunctionsTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/functions.tests'; |
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.
P2: The root-app spec imports rich-app function tests, which expect a different output structure (nested src/functions). This mismatches root-app’s root-level function outputs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-sdk/src/cli/__tests__/apps/root-app/__integration__/app-dev/app-dev.integration.spec.ts, line 5:
<comment>The root-app spec imports rich-app function tests, which expect a different output structure (nested src/functions). This mismatches root-app’s root-level function outputs.</comment>
<file context>
@@ -1,11 +1,11 @@
-import { defineManifestTests } from './tests/manifest.tests';
+import { type RunCliCommandResult } from '@/cli/__tests__/integration/utils/run-cli-command.util';
+import { defineFrontComponentsTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/front-components.tests';
+import { defineFunctionsTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/functions.tests';
+import { defineManifestTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/manifest.tests';
+import { runAppDev } from '@/cli/__tests__/integration/utils/run-app-dev.util';
</file context>
| import { defineFunctionsTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/functions.tests'; | |
| import { defineFunctionsTests } from '@/cli/__tests__/apps/root-app/__integration__/app-dev/tests/functions.tests'; |
| import { defineFunctionsTests } from './tests/functions.tests'; | ||
| import { defineManifestTests } from './tests/manifest.tests'; | ||
| import { type RunCliCommandResult } from '@/cli/__tests__/integration/utils/run-cli-command.util'; | ||
| import { defineFrontComponentsTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/front-components.tests'; |
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.
P2: The root-app spec now points to rich-app front-component tests, which assert a different output layout (src/components/*). This will fail against root-app’s root-level output and validates the wrong behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-sdk/src/cli/__tests__/apps/root-app/__integration__/app-dev/app-dev.integration.spec.ts, line 4:
<comment>The root-app spec now points to rich-app front-component tests, which assert a different output layout (src/components/*). This will fail against root-app’s root-level output and validates the wrong behavior.</comment>
<file context>
@@ -1,11 +1,11 @@
-import { defineFunctionsTests } from './tests/functions.tests';
-import { defineManifestTests } from './tests/manifest.tests';
+import { type RunCliCommandResult } from '@/cli/__tests__/integration/utils/run-cli-command.util';
+import { defineFrontComponentsTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/front-components.tests';
+import { defineFunctionsTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/functions.tests';
+import { defineManifestTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/manifest.tests';
</file context>
| import { defineFrontComponentsTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/front-components.tests'; | |
| import { defineFrontComponentsTests } from '@/cli/__tests__/apps/root-app/__integration__/app-dev/tests/front-components.tests'; |
| import { type RunCliCommandResult } from '@/cli/__tests__/integration/utils/run-cli-command.util'; | ||
| import { defineFrontComponentsTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/front-components.tests'; | ||
| import { defineFunctionsTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/functions.tests'; | ||
| import { defineManifestTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/manifest.tests'; |
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.
P2: The root-app integration spec now imports rich-app manifest tests, which assert a different application name and counts. This will validate the wrong manifest for root-app and likely fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-sdk/src/cli/__tests__/apps/root-app/__integration__/app-dev/app-dev.integration.spec.ts, line 6:
<comment>The root-app integration spec now imports rich-app manifest tests, which assert a different application name and counts. This will validate the wrong manifest for root-app and likely fail.</comment>
<file context>
@@ -1,11 +1,11 @@
+import { type RunCliCommandResult } from '@/cli/__tests__/integration/utils/run-cli-command.util';
+import { defineFrontComponentsTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/front-components.tests';
+import { defineFunctionsTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/functions.tests';
+import { defineManifestTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/manifest.tests';
+import { runAppDev } from '@/cli/__tests__/integration/utils/run-app-dev.util';
+import { defineConsoleOutputTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/console-output.tests';
</file context>
| import { defineManifestTests } from '@/cli/__tests__/apps/rich-app/__integration__/app-dev/tests/manifest.tests'; | |
| import { defineManifestTests } from '@/cli/__tests__/apps/root-app/__integration__/app-dev/tests/manifest.tests'; |
cc27285 to
83d5571
Compare
as title, upload built files to local storage