-
Notifications
You must be signed in to change notification settings - Fork 5k
Introduce asset watch #17378
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?
Introduce asset watch #17378
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.
1 issue found across 17 files
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-dev.ts">
<violation number="1" location="packages/twenty-sdk/src/cli/commands/app/app-dev.ts:210">
P2: When a component no longer has static assets, `assets` is `[]`, but this guard skips `updateManifestAssets`, leaving stale assets in the manifest. That can cause removed assets to remain in output and be uploaded. Update assets even when the list is empty so the manifest clears removed assets.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (updatedManifest && assets.length > 0) { | ||
| updatedManifest = updateManifestAssets({ | ||
| manifest: updatedManifest, | ||
| builtPath, | ||
| assets, | ||
| }); |
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: When a component no longer has static assets, assets is [], but this guard skips updateManifestAssets, leaving stale assets in the manifest. That can cause removed assets to remain in output and be uploaded. Update assets even when the list is empty so the manifest clears removed assets.
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/commands/app/app-dev.ts, line 210:
<comment>When a component no longer has static assets, `assets` is `[]`, but this guard skips `updateManifestAssets`, leaving stale assets in the manifest. That can cause removed assets to remain in output and be uploaded. Update assets even when the list is empty so the manifest clears removed assets.</comment>
<file context>
@@ -180,6 +185,43 @@ export class AppDevCommand {
+ checksum,
+ });
+
+ if (updatedManifest && assets.length > 0) {
+ updatedManifest = updateManifestAssets({
+ manifest: updatedManifest,
</file context>
| if (updatedManifest && assets.length > 0) { | |
| updatedManifest = updateManifestAssets({ | |
| manifest: updatedManifest, | |
| builtPath, | |
| assets, | |
| }); | |
| if (updatedManifest) { | |
| updatedManifest = updateManifestAssets({ | |
| manifest: updatedManifest, | |
| builtPath, | |
| assets, | |
| }) ?? updatedManifest; | |
| } |
Greptile SummaryThis PR adds asset watching functionality to the Twenty SDK CLI, enabling front components to bundle and track static assets (images, icons, etc.) during development and build processes. Key Changes:
Technical Implementation:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant AppDev as AppDevCommand
participant ManifestW as ManifestWatcher
participant FrontW as FrontComponentsWatcher
participant ESBuild as esbuild
participant Processor as esbuild-result-processor
participant ManifestB as manifest-build
Dev->>AppDev: Execute app dev command
AppDev->>ManifestB: runManifestBuild(appPath)
ManifestB-->>AppDev: Initial manifest + file paths
AppDev->>AppDev: Initialize file upload status
AppDev->>ManifestW: Start manifest watcher
AppDev->>FrontW: Start front components watcher
Note over FrontW: Configure esbuild with asset loaders
FrontW->>ESBuild: Create build context with file loader for static assets
ESBuild-->>FrontW: Build context ready
FrontW->>ESBuild: Initial rebuild
ESBuild->>ESBuild: Bundle components + copy assets
ESBuild-->>FrontW: Build result with metafile
FrontW->>FrontW: extractAssetsFromMetafile(result)
Note over FrontW: Parse metafile to find<br/>asset inputs and outputs
FrontW->>FrontW: Calculate asset checksums
FrontW-->>FrontW: Assets map created
FrontW->>Processor: processEsbuildResultWithAssets
Processor->>Processor: Check if checksums changed
Processor-->>FrontW: hasChanges, invoke callbacks
FrontW->>AppDev: onFileBuilt(builtPath, checksum, assets)
AppDev->>AppDev: updateFrontComponentFileStatus
AppDev->>ManifestB: updateManifestChecksum
AppDev->>ManifestB: updateManifestAssets
ManifestB-->>AppDev: Updated manifest
AppDev->>AppDev: Write manifest to output
Note over FrontW: Enter watch mode
FrontW->>ESBuild: Enable watch mode
Dev->>Dev: Modify component or asset
ESBuild->>FrontW: Detect change, trigger rebuild
FrontW->>FrontW: Extract assets from new build
FrontW->>Processor: Process results
Processor->>AppDev: Notify via onFileBuilt
AppDev->>AppDev: Update manifest with new checksums
|
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 (1)
-
packages/twenty-sdk/src/cli/utilities/build/front-components/front-component-watcher.ts, line 268-271 (link)logic:
.includes()check could match false positives if the extension appears anywhere in the path string
12 files reviewed, 1 comment
packages/twenty-sdk/src/cli/utilities/build/assets/asset-tracking-plugin.ts
Fixed
Show fixed
Hide fixed
packages/twenty-sdk/src/cli/utilities/build/front-components/front-component-watcher.ts
Show resolved
Hide resolved
| sourcePaths: buildResult.filePaths.functions, | ||
| watch: false, | ||
| onFileBuilt: (builtPath, checksum) => { | ||
| onFileBuilt: (builtPath: string, checksum: string, sourceAssetPaths: string[]) => { |
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.
record's arg
| for (const [_id, status] of this.state.fileStatusMaps.functions) { | ||
| if (status.builtPath === builtPath) { | ||
| status.checksum = checksum; | ||
| status.isUploaded = false; |
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.
mutation
| '.gif', | ||
| '.webp', | ||
| '.svg', | ||
| '.ico', |
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.
will also have to check the file encoding
No description provided.