feat: add ability to download deployed functions in debug mode#5657
feat: add ability to download deployed functions in debug mode#5657
Conversation
![propel-code-bot[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
TBonnin
left a comment
TBonnin
left a comment
There was a problem hiding this comment.
It downloads the typescript file, not the bundled js.
I would really prefer to get the bundled js. Most of the involved function are using some imported logic. Only having the ts entrypoint will be useless in those cases
|
|
||
| export const validationParams = z | ||
| .object({ | ||
| id: z.coerce.number().positive() |
There was a problem hiding this comment.
what about syncConfigId? it is not immediately clear which id we are talking about here
There was a problem hiding this comment.
Should probably leave it as id to keep consistency with other flow routes (/flows/:id/download). I'll rename it when destructuring thought to make it more clear
| return result.id; | ||
| } | ||
|
|
||
| async getProviderConfigKeyById(environment_id: number, id: number): Promise<string | null> { |
There was a problem hiding this comment.
let's have all new function return a Result
There was a problem hiding this comment.
Changes suggested to ensure correct file resolution and align download contents with TS-only intent.
Status: Changes Suggested | Risk: Medium
Issues Identified & Suggestions
- Use actual flow type for file path resolution:
local.service.ts - Exclude bundled JS if TS-only download is intended:
remote.service.ts
Review Details
📁 9 files reviewed | 💬 2 comments
Instruction Files
└── .claude/
├── agents/
│ └── nango-docs-migrator.md
└── skills/
├── agent-builder-skill/
│ ├── EXAMPLES.md
│ └── SKILL.md
├── creating-integration-docs/
│ └── SKILL.md
└── creating-skills-skill/
└── SKILL.md
👍 / 👎 individual comments to help improve reviews for you
|
|
||
| const scriptName = syncConfig.sync_name; | ||
|
|
||
| const jsFilePath = this.resolveIntegrationFile({ scriptType: 'sync', syncConfig, providerConfigKey }); |
There was a problem hiding this comment.
[Logic] The local download path hard-codes scriptType: 'sync'. For action flows this resolves to the syncs directory and the JS file lookup will fail. Use the flow’s actual type so actions resolve correctly.
| const jsFilePath = this.resolveIntegrationFile({ scriptType: 'sync', syncConfig, providerConfigKey }); | |
| const jsFilePath = this.resolveIntegrationFile({ scriptType: syncConfig.type, syncConfig, providerConfigKey }); |
Context for Agents
The local download path hard-codes `scriptType: 'sync'`. For action flows this resolves to the syncs directory and the JS file lookup will fail. Use the flow’s actual type so actions resolve correctly.
```suggestion
const jsFilePath = this.resolveIntegrationFile({ scriptType: syncConfig.type, syncConfig, providerConfigKey });
```
File: packages/shared/lib/services/file/local.service.ts
Line: 119| const { success: tsSuccess, error: tsError, response: tsFile } = await this.getStream(`${integrationFileLocation}/${scriptName}.ts`); | ||
| const scriptName = syncConfig.sync_name; | ||
|
|
||
| const jsFileLocation = syncConfig.file_location; |
There was a problem hiding this comment.
[Requirements] This adds the compiled .js artifact to the download zip. The PR intent states the download should be the TypeScript source only; this block will include the bundled JS and change the payload size/contents. If TS-only is the intended behavior, remove the JS file inclusion here (and the corresponding JS block in LocalFileService.zipAndSendFlow).
Context for Agents
This adds the compiled `.js` artifact to the download zip. The PR intent states the download should be the TypeScript source only; this block will include the bundled JS and change the payload size/contents. If TS-only is the intended behavior, remove the JS file inclusion here (and the corresponding JS block in `LocalFileService.zipAndSendFlow`).
File: packages/shared/lib/services/file/remote.service.ts
Line: 257| web.route('/flows/:id/disable').patch(webAuth, patchFlowDisable); | ||
| web.route('/flows/:id/enable').patch(webAuth, patchFlowEnable); | ||
| web.route('/flows/:id/frequency').patch(webAuth, patchFlowFrequency); | ||
| web.route('/flows/:id/download').post(webAuth, postFlowDownload); |
There was a problem hiding this comment.
do you know why it was a POST. Looks more like a GET to me?
| if (!func || !func.enabled || !func.id) { | ||
| return; | ||
| } | ||
| await apiFlowDownload(env, { id: func.id }, func.name); |
There was a problem hiding this comment.
what happen if apiFlowDownload fails?
2 participants
The new download flow also adds server-side parameter validation and provider config key resolution, streams a zip via local or remote file services, introduces a download webhook and API typing, and expands the bundle to include
nango.yamlplus both bundled JS and TypeScript sources when available.This summary was automatically generated by @propel-code-bot