Skip to content

Conversation

@danielva-monday
Copy link
Contributor

No description provided.

@danielva-monday danielva-monday changed the title [beta] add object storage add object storage Jun 30, 2025
Copy link
Contributor

@maorb-dev maorb-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, apart for some small improvements

@@ -1 +1 @@
export default { name: '@mondaycom/apps-sdk', version: '3.2.1' };
export default { name: '@mondaycom/apps-sdk', version: '3.3.0-beta.3' };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this also needed here?

Comment on lines 148 to 169
describe('listFiles', () => {
it('should list files with default options', async () => {
const result = await objectStorage.listFiles();

expect(result.success).toBe(true);
expect(Array.isArray(result.files)).toBe(true);
});

it('should list files with prefix filter', async () => {
const result = await objectStorage.listFiles({ prefix: 'test-' });

expect(result.success).toBe(true);
expect(Array.isArray(result.files)).toBe(true);
});

it('should list files with pagination', async () => {
const result = await objectStorage.listFiles({ maxResults: 10 });

expect(result.success).toBe(true);
expect(Array.isArray(result.files)).toBe(true);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the tests here are the same, they last 2 cases dont actually check the prefix filter and the limit.

Comment on lines 1 to 51
export type UploadFileOptions = {
contentType?: string;
metadata?: Record<string, string>;
}

export type UploadFileResponse = {
success: boolean;
fileName?: string;
fileUrl?: string;
error?: string;
}

export type DownloadFileResponse = {
success: boolean;
content?: Buffer;
contentType?: string;
error?: string;
}

export type DeleteFileResponse = {
success: boolean;
error?: string;
}

export type ListFilesOptions = {
prefix?: string;
maxResults?: number;
pageToken?: string;
}

export type FileInfo = {
name: string;
size: number;
contentType: string;
lastModified: Date;
etag: string;
metadata: Record<string, string>;
}

export type ListFilesResponse = {
success: boolean;
files?: Array<FileInfo>;
nextPageToken?: string;
error?: string;
}

export type GetFileInfoResponse = {
success: boolean;
fileInfo?: FileInfo;
error?: string;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's extract { success; boolean, error?: string } into a shared base response

Comment on lines 32 to 34
/**
* Upload a file to the object storage bucket
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all of these method comments are redundant

*/
async getFileInfo(fileName: string): Promise<GetFileInfoResponse> {
try {
const bucket: Bucket = this.storage.bucket(this.bucketName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be extracted to the class

Comment on lines 219 to 225
} catch (error) {
const errorMessage = error instanceof Error ? error.message : String(error);
const errorObj = error instanceof Error ? error : new Error(String(error));
logger.error('Failed to get file info:', { error: errorObj });
return {
success: false,
error: `Failed to get file info: ${errorMessage}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most of this can be extracted as well

Copy link
Contributor

@maorb-dev maorb-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

logger.error(`Failed to ${operation}:`, { error: errorObj });
return { errorMessage, errorObj };
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maorb-dev add another method that will provide the dev with a pre-signed url for uploading

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once added, also expose it with the mcode-sdk-api, simlar to this: https://github.com/DaPulse/mcode-sdk-api/pull/35/files
and also generate the python + PHP clients

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants