fix(cloud-image-editor): adjusted activityParams #906
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds validation to CloudImageEditorActivity to require Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Uploader as Uploader (upload flow)
participant Provider as UploadCtxProvider
participant API as UploaderPublicApi
participant Activity as CloudImageEditorActivity
participant UI as UI / Modal
Uploader->>Provider: emit file-upload-success(event with internalId)
Note right of Provider: getAPI() used by test to obtain API
Provider->>API: invoke setCurrentActivity('cloud-image-edit', {internalId})
API->>Activity: set current activity params
Activity->>Activity: validate params (requires internalId)
alt params valid
Activity->>UI: open cloud-image-editor modal
UI-->>Provider: modal visible
else invalid params
Activity-->>API: throw validation error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/blocks/CloudImageEditorActivity/CloudImageEditorActivity.ts (1)
18-30: The array handling is necessary due to a bug inUploaderPublicApi.setCurrentActivity, andExternalSourcehas the same unhandled vulnerability.Line 362 of
src/abstract/UploaderPublicApi.tsstores the rest parameter array directly in'*currentActivityParams'rather than unwrapping it withparams?.[0]. This causesActivityBlock.activityParamsto return an array instead of the expected object. CloudImageEditorActivity correctly handles this with array unwrapping, butExternalSourcedoes not—it will throw "External Source activity params not found" when called via the public API.Either fix
setCurrentActivityto unwrap the parameter, or add identical array handling toExternalSource. Additionally, add tests for both activities covering the public API path since none currently exist.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/blocks/CloudImageEditorActivity/CloudImageEditorActivity.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
src/blocks/CloudImageEditorActivity/CloudImageEditorActivity.ts
Outdated
Show resolved
Hide resolved
egordidenko
force-pushed
the
fix/cloud-image-edit-activity-params
branch
from
bcaed7c to
570a8ee
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the Cloud Image Editor activity where activityParams could be received in array format instead of as a direct object. This occurs when activity parameters are passed through the setCurrentActivity API, which uses rest parameters internally.
Key Changes:
- Added array format handling to extract the first parameter object from the array
- Added validation to throw an error if the array is empty
- Maintained backward compatibility with the existing object format
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/blocks/CloudImageEditorActivity/CloudImageEditorActivity.ts
Outdated
Show resolved
Hide resolved
src/blocks/CloudImageEditorActivity/CloudImageEditorActivity.ts
Outdated
Show resolved
Hide resolved
egordidenko
force-pushed
the
fix/cloud-image-edit-activity-params
branch
from
570a8ee to
3662a80
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/blocks/CloudImageEditorActivity/CloudImageEditorActivity.ts (1)
18-25: Add null/undefined guard before using theinoperator.The
inoperator at line 21 will throw aTypeErrorifparamsisnullorundefined. This can occur ifsuper.activityParamsreturns a nullish value.Additionally, based on the type signature in
UploaderPublicApi.setCurrentActivity(lines 354-358 of UploaderPublicApi.ts),paramsmay be passed as a tuple/array. Ifparamsis[{ internalId: '123' }], the check'internalId' in paramswill befalsebecause arrays don't have aninternalIdproperty directly—only their elements do. This inconsistency between the two files needs to be resolved (see my comment on UploaderPublicApi.ts).🔎 Proposed fix: Add defensive checks
override get activityParams(): ActivityParams { const params = super.activityParams; + + if (!params || typeof params !== 'object') { + throw new Error(`Cloud Image Editor activity params not found`); + } + + // Handle array format if needed (based on UploaderPublicApi signature) + const paramsObj = Array.isArray(params) ? params[0] : params; + + if (!paramsObj || !('internalId' in paramsObj)) { + throw new Error(`Cloud Image Editor activity params not found`); + } - if ('internalId' in params) { - return params; - } - throw new Error(`Cloud Image Editor activity params not found`); + return paramsObj; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/abstract/UploaderPublicApi.tssrc/blocks/CloudImageEditorActivity/CloudImageEditorActivity.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/abstract/UploaderPublicApi.ts
Outdated
| params: T extends keyof ActivityParamsMap | ||
| ? [ActivityParamsMap[T]] | ||
| : T extends RegisteredActivityType | ||
| ? [undefined?] | ||
| : [any?] | ||
| : [any?], |
There was a problem hiding this comment.
Removing the spread operator from the parameter changes the method signature in a way that breaks existing usage. The original ...params with type [ActivityParamsMap[T]] allowed calling the method with optional parameters like setCurrentActivity('camera') or setCurrentActivity('cloud-image-edit', { internalId: 'id' }). With the spread operator removed, the type signature now requires passing a tuple array, which would require calls like setCurrentActivity('cloud-image-edit', [{ internalId: 'id' }]). This breaks the existing API. Either restore the spread operator or change the parameter type to not be a tuple array.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/abstract/UploaderPublicApi.ts (1)
354-358: Tuple wrapping issue resolved.The previous review flagged that params was incorrectly wrapped in tuple format (
[ActivityParamsMap[T]]). This has been correctly fixed - the type now directly usesActivityParamsMap[T]without the array wrapper.
🧹 Nitpick comments (2)
tests/api.e2e.test.tsx (2)
103-103: Inconsistent API access pattern.Other tests in this file use
uploadCtxProvider.api(lines 30, 57, 83), but this test usesuploadCtxProvider.getAPI(). Consider using a consistent approach unless there's a specific reason for the difference.Suggested change for consistency
- const api = uploadCtxProvider.getAPI(); + const api = uploadCtxProvider.api;
109-110: Redundant type assertion.The
detailtype is already inferred fromCustomEvent<EventPayload['file-upload-success']>on line 109, making the assertion on line 110 unnecessary.Suggested simplification
const eventHandler = (event: CustomEvent<EventPayload['file-upload-success']>) => { - const detail = event.detail as EventPayload['file-upload-success']; + const detail = event.detail; api.setCurrentActivity('cloud-image-edit', { internalId: detail.internalId }); api.setModalState(true); };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/abstract/UploaderPublicApi.tstests/api.e2e.test.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
tests/api.e2e.test.tsx (1)
src/blocks/UploadCtxProvider/EventEmitter.ts (1)
EventPayload(43-67)
egordidenko
force-pushed
the
fix/cloud-image-edit-activity-params
branch
from
b5381f2 to
e95f1d7
Compare
egordidenko
force-pushed
the
fix/cloud-image-edit-activity-params
branch
2 times, most recently
from
1764b34 to
7723f2c
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
types/test/public-upload-api.test-d.tsx (1)
11-11: Pipeline failure caused by breaking signature change.The signature change in
UploaderPublicApi.setCurrentActivitynow requires a second parameter, causing this line to fail type checking. Activities like'camera'that don't require parameters must now explicitly passundefined.This should be addressed by making the
paramsparameter optional in the implementation (see review comment onsrc/abstract/UploaderPublicApi.ts).🔎 Temporary workaround
-api.setCurrentActivity('camera'); +api.setCurrentActivity('camera', undefined);However, the proper fix is to make the parameter optional in the method signature.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/abstract/UploaderPublicApi.tstypes/test/public-upload-api.test-d.tsx
🧰 Additional context used
🪛 GitHub Actions: checks
types/test/public-upload-api.test-d.tsx
[error] 11-11: tsd type check failed: Expected 2 arguments, but got 1. (Step: tsd -t ./dist/index.d.ts -f ./types/test/)
🪛 GitHub Check: CodeQL
types/test/public-upload-api.test-d.tsx
[notice] 3-3: Unused variable, import, function or class
Unused import createRunnableDevEnvironment.
🔇 Additional comments (2)
types/test/public-upload-api.test-d.tsx (2)
18-18: Test correctly verifies required params.This test appropriately validates that
cloud-image-editactivity cannot be called withundefinedparams since it requiresinternalId.
36-36: Test correctly validates custom activity flexibility.This appropriately verifies that custom activities can be called with
undefinedparams, demonstrating the type system's flexibility for unregistered activities.
egordidenko
force-pushed
the
fix/cloud-image-edit-activity-params
branch
from
7723f2c to
ba03d4e
Compare
egordidenko
force-pushed
the
fix/cloud-image-edit-activity-params
branch
from
ba03d4e to
4e08d5a
Compare
Description
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.