feat(metadata-view): Bulk download logic only #4237
Conversation
WalkthroughListen up! This PR adds a new ZipDownload API using the Box TypeScript SDK, wires it into APIFactory, updates Jest config/setup for TextDecoder and jose transforms, tweaks build script flags, adjusts @box/metadata-view versions, and introduces unit tests for the new ZipDownload flow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Caller
participant AF as APIFactory
participant ZD as ZipDownloadAPI
participant SDK as Box SDK (box-typescript-sdk-gen)
participant Br as Browser
Dev->>AF: getZipDownloadAPI()
AF-->>Dev: ZipDownloadAPI instance
Dev->>ZD: createZipDownload(items)
ZD->>SDK: createZipDownload(request)
alt Response has downloadUrl
SDK-->>ZD: { downloadUrl, ... }
ZD->>Br: window.open(downloadUrl)
ZD-->>Dev: ZipDownloadResponse
else In progress / no URL / failed
SDK-->>ZD: { state / no downloadUrl }
ZD-->>Dev: ZipDownloadResponse
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
scripts/jest/jest.config.js (1)
29-30: Transpile jose from node_modules to fix ESM/test issues — aligns with prior feedbackWhitelisting jose in transformIgnorePatterns is the right call given the SDK’s transitive deps. Keep an eye on test perf; if needed, narrow the pattern to jose($|/).
🧹 Nitpick comments (1)
scripts/jest/jest-setup.ts (1)
27-30: Guard TextDecoder definition to avoid redefining in newer environmentsMinor: On Node 18+/jsdom, TextDecoder may already exist. Guard the define to avoid potential redefinition issues.
Apply this diff:
-Object.defineProperty(global, 'TextDecoder', { - value: util.TextDecoder, -}); +if (!('TextDecoder' in global)) { + Object.defineProperty(global, 'TextDecoder', { + value: util.TextDecoder, + }); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
package.json(1 hunks)scripts/jest/jest-setup.ts(1 hunks)scripts/jest/jest.config.js(1 hunks)src/api/APIFactory.js(3 hunks)src/api/ZipDownload.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/api/APIFactory.js (1)
src/api/ZipDownload.ts (1)
ZipDownloadAPI(33-102)
🪛 Biome (2.1.2)
src/api/APIFactory.js
[error] 208-208: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 864-864: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
🪛 ESLint
src/api/ZipDownload.ts
[error] 85-85: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
⏰ 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). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (5)
package.json (1)
361-364: New runtime dependency: validate package name/version and bundle impactAdding box-typescript-sdk-gen as a runtime dependency looks appropriate if the SDK is used in shipped code. Please verify:
- The package name and version range are correct for the API surface you’re calling.
- Bundle/test impact: jose is a transitive dep and can increase test time and bundle size; you’ve whitelisted it in Jest, which is good. Consider measuring bundle impact before and after.
If consumers of box-ui-elements don’t need to import the SDK directly, keep it in dependencies (not peer). If we only use it internally during development, move it to devDependencies.
Would you like me to add a quick size-check script (bundle analyzer) to CI to track the delta?
src/api/ZipDownload.ts (2)
45-50: Confirm appropriate auth class; avoid Dev Token auth in productionBoxDeveloperTokenAuth is typically intended for dev tokens. For production, prefer the appropriate bearer/OAuth auth class expected by the SDK for standard access tokens. If DeveloperTokenAuth is just a thin bearer wrapper in this SDK, please confirm. Otherwise, switch to the production-safe auth.
Would you like me to check the SDK docs and update this to the recommended auth for user tokens?
62-67: Field name alignment with SDK/APIVerify whether the SDK expects downloadFileName or download_file_name. If the SDK maps camelCase to snake_case, this is fine; otherwise, the field may be ignored.
If needed, I can update the request shape once confirmed.
src/api/APIFactory.js (2)
39-39: New API import is fineThe import of ZipDownload integrates cleanly with the existing API surface.
864-867: Flow vs TypeScript return type hint false positive from static analysisBiome’s parse errors about return types are false positives in this Flow-typed file; Flow supports method return annotations. No action needed.
| zipDownloadAPI: ZipDownloadAPI; | ||
|
|
There was a problem hiding this comment.
Add destroy handling for zipDownloadAPI to prevent leaks
APIFactory.destroy currently does not clear zipDownloadAPI, unlike other APIs. Add symmetrical cleanup to avoid retaining instances/options.
Apply this diff near the other cleanup blocks in destroy():
if (this.intelligenceAPI) {
this.intelligenceAPI.destroy();
delete this.intelligenceAPI;
}
+
+ if (this.zipDownloadAPI) {
+ // No-op if destroy is not implemented
+ if (typeof this.zipDownloadAPI.destroy === 'function') {
+ this.zipDownloadAPI.destroy();
+ }
+ delete this.zipDownloadAPI;
+ }And consider adding a no-op destroy() method to ZipDownloadAPI for consistency.
I can add the no-op destroy() to ZipDownloadAPI if you want.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| zipDownloadAPI: ZipDownloadAPI; | |
| if (this.intelligenceAPI) { | |
| this.intelligenceAPI.destroy(); | |
| delete this.intelligenceAPI; | |
| } | |
| if (this.zipDownloadAPI) { | |
| // No-op if destroy() is not implemented | |
| if (typeof this.zipDownloadAPI.destroy === 'function') { | |
| this.zipDownloadAPI.destroy(); | |
| } | |
| delete this.zipDownloadAPI; | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 208-208: return types can only be used in TypeScript files
remove this type annotation
(parse)
🤖 Prompt for AI Agents
In src/api/APIFactory.js around lines 208-209, APIFactory.destroy() currently
omits cleanup for zipDownloadAPI; add the same symmetrical teardown as other
APIs by calling zipDownloadAPI.destroy() if present (or safe-call) and then
setting this.zipDownloadAPI = undefined/null inside the destroy() method
alongside the other cleanup blocks; also add a no-op destroy() method to the
ZipDownloadAPI class (or confirm one exists) so the safe-call is valid and
consistent.
| try { | ||
| // Create the ZIP download using the Box SDK | ||
| const zipDownload = await this.client.zipDownloads.createZipDownload(zipRequest); | ||
| this.downloadZipFile(zipDownload.downloadUrl); | ||
|
|
||
| return zipDownload as unknown as ZipDownloadResponse; | ||
| } catch (error: unknown) { | ||
| const errorMessage = error instanceof Error ? error.message : String(error); | ||
| throw new Error(`Failed to create ZIP download: ${errorMessage}`); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Side effects in API layer and readiness of downloadUrl
Immediately opening the download URL couples transport logic with UI and assumes the URL is ready. For large zips, you often need to poll the statusUrl until state === 'succeeded'. Recommend returning the response and letting the caller decide when/how to download, or add an opt-in flag.
Apply this diff to make auto-download optional and safer:
- async createZipDownload(items: ZipDownloadItem[]): Promise<ZipDownloadResponse> {
+ async createZipDownload(
+ items: ZipDownloadItem[],
+ opts?: { autoDownload?: boolean }
+ ): Promise<ZipDownloadResponse> {
@@
- try {
+ try {
// Create the ZIP download using the Box SDK
const zipDownload = await this.client.zipDownloads.createZipDownload(zipRequest);
- this.downloadZipFile(zipDownload.downloadUrl);
+ const autoDownload = opts?.autoDownload ?? false;
+ if (autoDownload && zipDownload.downloadUrl) {
+ this.downloadZipFile(zipDownload.downloadUrl);
+ }
return zipDownload as unknown as ZipDownloadResponse;Optionally, add follow-up polling in the caller using statusUrl before invoking downloadZipFile.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| // Create the ZIP download using the Box SDK | |
| const zipDownload = await this.client.zipDownloads.createZipDownload(zipRequest); | |
| this.downloadZipFile(zipDownload.downloadUrl); | |
| return zipDownload as unknown as ZipDownloadResponse; | |
| } catch (error: unknown) { | |
| const errorMessage = error instanceof Error ? error.message : String(error); | |
| throw new Error(`Failed to create ZIP download: ${errorMessage}`); | |
| // In src/api/ZipDownload.ts | |
| // Before: | |
| // async createZipDownload(items: ZipDownloadItem[]): Promise<ZipDownloadResponse> { | |
| // try { | |
| // // Create the ZIP download using the Box SDK | |
| // const zipDownload = await this.client.zipDownloads.createZipDownload(zipRequest); | |
| // this.downloadZipFile(zipDownload.downloadUrl); | |
| // | |
| // return zipDownload as unknown as ZipDownloadResponse; | |
| // } catch (error: unknown) { | |
| // const errorMessage = error instanceof Error ? error.message : String(error); | |
| // throw new Error(`Failed to create ZIP download: ${errorMessage}`); | |
| // } | |
| // } | |
| // After: | |
| async createZipDownload( | |
| items: ZipDownloadItem[], | |
| opts?: { autoDownload?: boolean } | |
| ): Promise<ZipDownloadResponse> { | |
| try { | |
| // Create the ZIP download using the Box SDK | |
| const zipDownload = await this.client.zipDownloads.createZipDownload(zipRequest); | |
| const autoDownload = opts?.autoDownload ?? false; | |
| if (autoDownload && zipDownload.downloadUrl) { | |
| this.downloadZipFile(zipDownload.downloadUrl); | |
| } | |
| return zipDownload as unknown as ZipDownloadResponse; | |
| } catch (error: unknown) { | |
| const errorMessage = error instanceof Error ? error.message : String(error); | |
| throw new Error(`Failed to create ZIP download: ${errorMessage}`); | |
| } | |
| } |
| /** | ||
| * Download the ZIP file bytestream to the user's device using window.open | ||
| * @param bytestream - The ZIP file data as a bytestream | ||
| * @param filename - The name for the downloaded file | ||
| */ | ||
| private downloadZipFile(url: any): void { | ||
| try { | ||
| // Open in new tab - user can save from there | ||
| window.open(url, '_blank', 'noopener,noreferrer'); | ||
|
|
||
| window.focus(); | ||
|
|
||
| // Clean up after a delay to allow the download to start | ||
| setTimeout(() => { | ||
| URL.revokeObjectURL(url); | ||
| }, 1000); | ||
| } catch (error: unknown) { | ||
| const errorMessage = error instanceof Error ? error.message : String(error); | ||
| console.error('Failed to download ZIP file:', errorMessage); | ||
| throw new Error(`Failed to download ZIP file: ${errorMessage}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix type any, handle popup blockers, and avoid revoking non-blob URLs
- Type the URL as string to satisfy lint.
- window.open may return null if blocked; focus that window only when present; otherwise, fall back to location.assign.
- URL.revokeObjectURL should only be used for blob: URLs created via URL.createObjectURL. Calling it on https URLs will throw inside the setTimeout (outside your try/catch).
Apply this diff:
- private downloadZipFile(url: any): void {
+ private downloadZipFile(url: string): void {
try {
- // Open in new tab - user can save from there
- window.open(url, '_blank', 'noopener,noreferrer');
-
- window.focus();
-
- // Clean up after a delay to allow the download to start
- setTimeout(() => {
- URL.revokeObjectURL(url);
- }, 1000);
+ // Open in new tab - user can save from there
+ const win = window.open(url, '_blank', 'noopener,noreferrer');
+ if (win && typeof win.focus === 'function') {
+ win.focus();
+ } else {
+ // Popup blocked; fall back to same-tab navigation
+ window.location.assign(url);
+ }
+ // Only revoke if this is a blob URL we created (not expected in current flow)
+ // if (url.startsWith('blob:')) {
+ // setTimeout(() => URL.revokeObjectURL(url), 1000);
+ // }
} catch (error: unknown) {
const errorMessage = error instanceof Error ? error.message : String(error);
console.error('Failed to download ZIP file:', errorMessage);
throw new Error(`Failed to download ZIP file: ${errorMessage}`);
}
}Also update the JSDoc above to refer to url instead of bytestream/filename.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Download the ZIP file bytestream to the user's device using window.open | |
| * @param bytestream - The ZIP file data as a bytestream | |
| * @param filename - The name for the downloaded file | |
| */ | |
| private downloadZipFile(url: any): void { | |
| try { | |
| // Open in new tab - user can save from there | |
| window.open(url, '_blank', 'noopener,noreferrer'); | |
| window.focus(); | |
| // Clean up after a delay to allow the download to start | |
| setTimeout(() => { | |
| URL.revokeObjectURL(url); | |
| }, 1000); | |
| } catch (error: unknown) { | |
| const errorMessage = error instanceof Error ? error.message : String(error); | |
| console.error('Failed to download ZIP file:', errorMessage); | |
| throw new Error(`Failed to download ZIP file: ${errorMessage}`); | |
| } | |
| } | |
| /** | |
| * Download the ZIP file at the specified URL to the user's device using window.open. | |
| * @param url - The URL of the ZIP file to download. | |
| */ | |
| private downloadZipFile(url: string): void { | |
| try { | |
| // Open in new tab - user can save from there | |
| const win = window.open(url, '_blank', 'noopener,noreferrer'); | |
| if (win && typeof win.focus === 'function') { | |
| win.focus(); | |
| } else { | |
| // Popup blocked; fall back to same-tab navigation | |
| window.location.assign(url); | |
| } | |
| // Only revoke if this is a blob URL we created (not expected in current flow) | |
| // if (url.startsWith('blob:')) { | |
| // setTimeout(() => URL.revokeObjectURL(url), 1000); | |
| // } | |
| } catch (error: unknown) { | |
| const errorMessage = error instanceof Error ? error.message : String(error); | |
| console.error('Failed to download ZIP file:', errorMessage); | |
| throw new Error(`Failed to download ZIP file: ${errorMessage}`); | |
| } | |
| } |
🧰 Tools
🪛 ESLint
[error] 85-85: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🤖 Prompt for AI Agents
In src/api/ZipDownload.ts around lines 80 to 101, update the JSDoc to reference
the url param (not bytestream/filename), change the downloadZipFile signature to
accept url: string, and guard window.open results: call window.open(url,
'_blank', 'noopener,noreferrer') and if it returns a Window object call
win.focus(); otherwise fall back to window.location.assign(url); finally only
call URL.revokeObjectURL(url) inside the timeout if url startsWith('blob:') to
avoid revoking non-blob URLs and throwing.
package.json
Outdated
| "dependencies": { | ||
| "box-typescript-sdk-gen": "^1.17.1" | ||
| } |
There was a problem hiding this comment.
nit: i would move this the dependencies section to be near peer and dev. either put it before devDependencies or after peerDependencies
greg-in-a-box
force-pushed
the
download
branch
2 times, most recently
from
ec6b379 to
8d791eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/api/APIFactory.js (2)
208-209: Add symmetrical destroy/cleanup for zipDownloadAPI
destroy()doesn’t tear downzipDownloadAPI, which can retain options/instances across navigations in a long‑lived SPA.Apply this diff near the other cleanup blocks in
destroy():if (this.intelligenceAPI) { this.intelligenceAPI.destroy(); delete this.intelligenceAPI; } + + if (this.zipDownloadAPI) { + if (typeof this.zipDownloadAPI.destroy === 'function') { + this.zipDownloadAPI.destroy(); + } + delete this.zipDownloadAPI; + }Optional companion change outside this file (ZipDownload.ts): add a no‑op
destroy()for parity (see suggestion in the test file comment).
864-867: Require a concrete token (string) for ZipDownload; avoid passing function tokens through
APIOptions.tokenallowsstring|function, butZipDownloadAPIexpects astring. Passingthis.optionsdirectly risks a runtime failure when a function token is supplied (a common pattern in this codebase).Apply this minimal, explicit API change so callers provide a concrete token (and optional filename) up front:
- getZipDownloadAPI(): ZipDownloadAPI { - this.zipDownloadAPI = new ZipDownloadAPI(this.options); - return this.zipDownloadAPI; - } + getZipDownloadAPI(token: string, opts?: { downloadFileName?: string }): ZipDownloadAPI { + this.zipDownloadAPI = new ZipDownloadAPI({ + token, + downloadFileName: opts?.downloadFileName, + }); + return this.zipDownloadAPI; + }If you prefer not to change the public method shape, an alternative is to resolve sync function tokens in place (throw on async/Promise results). I can provide that patch as well.
Run to check for call sites that need updating:
#!/bin/bash # Find usages of getZipDownloadAPI rg -nC2 '\bgetZipDownloadAPI\s*\(' --type=js --type=ts
🧹 Nitpick comments (2)
scripts/jest/jest-setup.ts (1)
27-29: Harden TextDecoder polyfill: add guards and make it configurable/writablePrevent accidental redefinition errors and support code that reads from
window.TextDecodertoo.Apply this diff:
-Object.defineProperty(global, 'TextDecoder', { - value: util.TextDecoder, -}); +if (!('TextDecoder' in global)) { + Object.defineProperty(global, 'TextDecoder', { + value: util.TextDecoder, + writable: true, + configurable: true, + }); +} +if (!('TextDecoder' in window)) { + Object.defineProperty(window as any, 'TextDecoder', { + value: util.TextDecoder, + writable: true, + configurable: true, + }); +}src/api/__tests__/ZipDownload.test.ts (1)
31-56: Optional: add a no-op destroy() to ZipDownloadAPI for factory parityAPIFactory will now clean up
zipDownloadAPI. Adding a harmlessdestroy()keeps patterns consistent and avoids optional chaining everywhere.Outside this file (in
src/api/ZipDownload.ts), add:// at the end of the class public destroy(): void { // No-op; keep for APIFactory symmetry // Null out large refs to help GC in long-lived pages // @ts-expect-error deliberate nulling for GC this.client = null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
package.json(1 hunks)scripts/jest/jest-setup.ts(1 hunks)scripts/jest/jest.config.js(1 hunks)src/api/APIFactory.js(3 hunks)src/api/ZipDownload.ts(1 hunks)src/api/__tests__/ZipDownload.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/api/ZipDownload.ts
- scripts/jest/jest.config.js
- package.json
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: jpan-box
PR: box/box-ui-elements#4237
File: src/api/ZipDownload.ts:6-8
Timestamp: 2025-08-21T19:19:44.538Z
Learning: For Box TypeScript SDK integrations, jpan-box prefers deep imports (e.g., from box-typescript-sdk-gen/lib/schemas/...generated.d.ts.js) over defining local types, citing that Box v2 APIs don't typically have breaking changes, making the deep import approach more viable despite potential brittleness.
🧬 Code graph analysis (2)
src/api/__tests__/ZipDownload.test.ts (1)
src/api/ZipDownload.ts (4)
ZipDownloadAPI(33-104)ZipDownloadOptions(14-17)ZipDownloadItem(9-12)ZipDownloadResponse(19-27)
src/api/APIFactory.js (1)
src/api/ZipDownload.ts (1)
ZipDownloadAPI(33-104)
🪛 Biome (2.1.2)
src/api/APIFactory.js
[error] 208-208: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 864-864: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ 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: Summary
🔇 Additional comments (1)
src/api/APIFactory.js (1)
39-39: Import looks correctImporting the default
ZipDownloadAPIvalue is appropriate since it’s instantiated in this file, and it aligns with the team’s preference to rely on the generated SDK for types/clients. Nothing to change here.
| Object.defineProperty(global, 'URL', { | ||
| value: { | ||
| revokeObjectURL: mockUrlRevokeObjectURL, | ||
| }, | ||
| writable: true, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t replace the global URL object; spy on revokeObjectURL instead
Overwriting global.URL to a stub removes other URL APIs and can break unrelated tests. Prefer spying on the specific method.
Apply this diff:
-Object.defineProperty(global, 'URL', {
- value: {
- revokeObjectURL: mockUrlRevokeObjectURL,
- },
- writable: true,
-});
+jest.spyOn(global.URL, 'revokeObjectURL').mockImplementation(mockUrlRevokeObjectURL as any);Then, add teardown to restore after each test (outside the shown hunk):
afterEach(() => {
(global.URL.revokeObjectURL as jest.Mock | undefined)?.mockRestore?.();
});🤖 Prompt for AI Agents
In src/api/__tests__/ZipDownload.test.ts around lines 24 to 29, don’t overwrite
global.URL; instead spy on the revokeObjectURL method and mock its
implementation (e.g., jest.spyOn(global.URL,
'revokeObjectURL').mockImplementation(...)) so other URL APIs remain intact, and
add an afterEach teardown that restores the spy (calling mockRestore() if
present) to clean up after each test.
| test('should revoke object URL after timeout', async () => { | ||
| jest.useFakeTimers(); | ||
|
|
||
| mockCreateZipDownload.mockResolvedValue(mockZipDownloadResponse); | ||
|
|
||
| await zipDownloadAPI.createZipDownload(mockItems); | ||
|
|
||
| // Fast-forward timers to trigger the setTimeout | ||
| jest.advanceTimersByTime(1000); | ||
|
|
||
| expect(mockUrlRevokeObjectURL).toHaveBeenCalledWith( | ||
| 'https://api.box.com/2.0/zip_downloads/test-download-url', | ||
| ); | ||
|
|
||
| jest.useRealTimers(); | ||
| }); |
There was a problem hiding this comment.
Revisit expectation: don’t revoke non-blob URLs (revoking https: will throw in browsers)
URL.revokeObjectURL is only valid for blob: URLs. The production code calls it for whatever URL the API returns (usually https:), which would raise an uncaught error in the setTimeout callback.
Update the production code to guard the call, and adjust this test to assert no revoke for https: URLs.
Update (ZipDownload.ts) — outside this file:
- setTimeout(() => {
- URL.revokeObjectURL(url);
- }, 1000);
+ setTimeout(() => {
+ if (typeof url === 'string' && url.startsWith('blob:')) {
+ try {
+ URL.revokeObjectURL(url);
+ } catch {
+ /* no-op */
+ }
+ }
+ }, 1000);Update this test hunk:
- test('should revoke object URL after timeout', async () => {
+ test('should not revoke non-blob URL after timeout', async () => {
jest.useFakeTimers();
mockCreateZipDownload.mockResolvedValue(mockZipDownloadResponse);
await zipDownloadAPI.createZipDownload(mockItems);
// Fast-forward timers to trigger the setTimeout
jest.advanceTimersByTime(1000);
- expect(mockUrlRevokeObjectURL).toHaveBeenCalledWith(
- 'https://api.box.com/2.0/zip_downloads/test-download-url',
- );
+ expect(mockUrlRevokeObjectURL).not.toHaveBeenCalled();
jest.useRealTimers();
});Optionally, add a separate test that feeds a blob: URL and asserts the revoke call.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('should revoke object URL after timeout', async () => { | |
| jest.useFakeTimers(); | |
| mockCreateZipDownload.mockResolvedValue(mockZipDownloadResponse); | |
| await zipDownloadAPI.createZipDownload(mockItems); | |
| // Fast-forward timers to trigger the setTimeout | |
| jest.advanceTimersByTime(1000); | |
| expect(mockUrlRevokeObjectURL).toHaveBeenCalledWith( | |
| 'https://api.box.com/2.0/zip_downloads/test-download-url', | |
| ); | |
| jest.useRealTimers(); | |
| }); | |
| test('should not revoke non-blob URL after timeout', async () => { | |
| jest.useFakeTimers(); | |
| mockCreateZipDownload.mockResolvedValue(mockZipDownloadResponse); | |
| await zipDownloadAPI.createZipDownload(mockItems); | |
| // Fast-forward timers to trigger the setTimeout | |
| jest.advanceTimersByTime(1000); | |
| expect(mockUrlRevokeObjectURL).not.toHaveBeenCalled(); | |
| jest.useRealTimers(); | |
| }); |
🤖 Prompt for AI Agents
In src/api/__tests__/ZipDownload.test.ts around lines 197 to 212, the test
currently expects URL.revokeObjectURL to be called for an https: URL which is
invalid; update the production code (src/api/ZipDownload.ts) so the setTimeout
callback only calls URL.revokeObjectURL when the returned URL begins with
"blob:" (e.g., if (url && url.startsWith('blob:')) URL.revokeObjectURL(url)),
then modify this test to assert that revokeObjectURL is NOT called for the
https:// test URL, and optionally add a new test case that supplies a blob: URL
and asserts revokeObjectURL is called.
| test('should handle non-Error window.open errors', async () => { | ||
| mockWindowOpen.mockImplementation(() => { | ||
| throw new Error('String window error'); | ||
| }); | ||
|
|
||
| mockCreateZipDownload.mockResolvedValue(mockZipDownloadResponse); | ||
|
|
||
| await expect(zipDownloadAPI.createZipDownload(mockItems)).rejects.toThrow( | ||
| 'Failed to download ZIP file: String window error', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix test to actually throw a non-Error value
The test name says “non-Error” but it throws an Error. Throw a primitive/string to exercise the non-Error path.
Apply this diff:
- mockWindowOpen.mockImplementation(() => {
- throw new Error('String window error');
- });
+ mockWindowOpen.mockImplementation(() => {
+ // Intentionally throw a non-Error
+ // eslint-disable-next-line no-throw-literal
+ throw 'String window error';
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('should handle non-Error window.open errors', async () => { | |
| mockWindowOpen.mockImplementation(() => { | |
| throw new Error('String window error'); | |
| }); | |
| mockCreateZipDownload.mockResolvedValue(mockZipDownloadResponse); | |
| await expect(zipDownloadAPI.createZipDownload(mockItems)).rejects.toThrow( | |
| 'Failed to download ZIP file: String window error', | |
| ); | |
| }); | |
| test('should handle non-Error window.open errors', async () => { | |
| mockWindowOpen.mockImplementation(() => { | |
| // Intentionally throw a non-Error | |
| // eslint-disable-next-line no-throw-literal | |
| throw 'String window error'; | |
| }); | |
| mockCreateZipDownload.mockResolvedValue(mockZipDownloadResponse); | |
| await expect(zipDownloadAPI.createZipDownload(mockItems)).rejects.toThrow( | |
| 'Failed to download ZIP file: String window error', | |
| ); | |
| }); |
🤖 Prompt for AI Agents
In src/api/__tests__/ZipDownload.test.ts around lines 227 to 237, the test
labeled “should handle non-Error window.open errors” incorrectly throws an Error
object; change the mockWindowOpen.mockImplementation to throw a primitive (e.g.,
throw 'String window error';) so the non-Error code path is exercised and the
existing expectation remains valid.
c919226 to
156f010
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/api/ZipDownload.ts (2)
57-76: Decouple I/O side effect; make auto-download opt-in and avoid race with async generationcreateZipDownload currently auto-opens the download URL. This couples transport with UI and risks opening before state === 'succeeded' for large zips. Recommend an opt-in flag and letting callers poll statusUrl when needed.
- async createZipDownload(items: ZipDownloadItem[]): Promise<ZipDownloadResponse> { + async createZipDownload( + items: ZipDownloadItem[], + opts?: { autoDownload?: boolean } + ): Promise<ZipDownloadResponse> { @@ - // Create the ZIP download using the Box SDK + // Create the ZIP download using the Box SDK const zipDownload = await this.client.zipDownloads.createZipDownload(zipRequest); - // Only download if we have a download URL - if (zipDownload.downloadUrl) { - this.downloadZipFile(zipDownload.downloadUrl); - } + const autoDownload = opts?.autoDownload ?? false; + if (autoDownload && zipDownload.downloadUrl) { + this.downloadZipFile(zipDownload.downloadUrl); + }
84-103: Guard popup blockers, focus the correct window, and don’t revoke non-blob URLs
- window.open can return null; focus only if a Window is returned.
- Focus the opened window, not the global.
- URL.revokeObjectURL must only be used for blob: URLs you created; calling it on https URLs throws asynchronously.
- private downloadZipFile(url: string): void { + private downloadZipFile(url: string): void { try { - // Open in new tab - user can save from there - window.open(url, '_blank', 'noopener,noreferrer'); - - window.focus(); - - // Clean up after a delay to allow the download to start - setTimeout(() => { - URL.revokeObjectURL(url); - }, 1000); + // Open in new tab - user can save from there + const win = window.open(url, '_blank', 'noopener,noreferrer'); + if (win && typeof win.focus === 'function') { + win.focus(); + } else { + // Popup blocked; fall back to same-tab navigation + window.location.assign(url); + } + // Only revoke if this is a blob URL created via URL.createObjectURL + // if (url.startsWith('blob:')) { + // setTimeout(() => URL.revokeObjectURL(url), 1000); + // } } catch (error: unknown) { const errorMessage = error instanceof Error ? error.message : String(error); throw new Error(`Failed to download ZIP file: ${errorMessage}`); } }src/api/APIFactory.js (2)
380-388: Tear down zipDownloadAPI in destroy() to prevent instance leaksMissing symmetric cleanup for zipDownloadAPI. Add a guarded teardown like the others.
if (this.intelligenceAPI) { this.intelligenceAPI.destroy(); delete this.intelligenceAPI; } + + if (this.zipDownloadAPI) { + if (typeof this.zipDownloadAPI.destroy === 'function') { + this.zipDownloadAPI.destroy(); + } + delete this.zipDownloadAPI; + } if (destroyCache) { this.options.cache = new Cache(); }
865-868: Resolve token type mismatch: require a concrete string token (or adapt to function/async)APIOptions.token allows string|function, but ZipDownloadAPI expects a string and constructs auth immediately. Passing a function here will throw at runtime.
Two options:
- Minimal: require a concrete string token param for this getter (shown below).
- Broader: teach ZipDownloadAPI to resolve function/async tokens before calling the SDK.
-getZipDownloadAPI(): ZipDownloadAPI { - this.zipDownloadAPI = new ZipDownloadAPI(this.options); - return this.zipDownloadAPI; -} +getZipDownloadAPI(token: string, opts?: { downloadFileName?: string }): ZipDownloadAPI { + this.zipDownloadAPI = new ZipDownloadAPI({ + token, + downloadFileName: opts?.downloadFileName, + }); + return this.zipDownloadAPI; +}If you prefer the factory to accept string|function and resolve here (including async), I can draft an adapter that normalizes token acquisition without changing the ZipDownloadAPI signature.
🧹 Nitpick comments (3)
scripts/jest/jest-setup.ts (1)
27-30: Guard TextDecoder shim to avoid redefining on newer Node/JSDOMOn some Node/JSDOM versions TextDecoder already exists and may be non-configurable, causing defineProperty to throw. Guard the shim.
-Object.defineProperty(global, 'TextDecoder', { - value: util.TextDecoder, -}); +if (!('TextDecoder' in global)) { + Object.defineProperty(global, 'TextDecoder', { + value: util.TextDecoder, + }); +}If jose (or the SDK) needs WebCrypto, consider adding a guarded global crypto shim as well:
// Add near other shims if (!global.crypto) { // eslint-disable-next-line global-require // @ts-ignore global.crypto = require('crypto').webcrypto; }src/api/ZipDownload.ts (2)
6-8: Make ZipDownloadRequest a type-only import to prevent accidental runtime importKeep the deep import per team preference, but mark it as type-only so bundlers/Jest never attempt to load a .d.ts-backed path at runtime.
-import { BoxClient, BoxDeveloperTokenAuth } from 'box-typescript-sdk-gen'; -import { ZipDownloadRequest } from 'box-typescript-sdk-gen/lib/schemas/zipDownloadRequest.generated.d.ts.js'; +import { BoxClient, BoxDeveloperTokenAuth } from 'box-typescript-sdk-gen'; +import type { ZipDownloadRequest } from 'box-typescript-sdk-gen/lib/schemas/zipDownloadRequest.generated.d.ts.js';
33-50: Consider adding a no-op destroy() for symmetry with APIFactory.destroyAPIFactory.destroy tears down API instances; adding a no-op destroy keeps behavior consistent and allows future resource cleanup without API churn.
export default class ZipDownloadAPI { private client: BoxClient; private options: ZipDownloadOptions; @@ constructor(options: ZipDownloadOptions) { this.options = options; @@ }); } + + /** + * No-op destroy for APIFactory symmetry and future extensibility. + */ + destroy(): void {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
package.json(1 hunks)scripts/jest/jest-setup.ts(1 hunks)scripts/jest/jest.config.js(1 hunks)src/api/APIFactory.js(3 hunks)src/api/ZipDownload.ts(1 hunks)src/api/__tests__/ZipDownload.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/api/tests/ZipDownload.test.ts
- package.json
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jpan-box
PR: box/box-ui-elements#4237
File: src/api/ZipDownload.ts:6-8
Timestamp: 2025-08-21T19:19:44.538Z
Learning: For Box TypeScript SDK integrations, jpan-box prefers deep imports (e.g., from box-typescript-sdk-gen/lib/schemas/...generated.d.ts.js) over defining local types, citing that Box v2 APIs don't typically have breaking changes, making the deep import approach more viable despite potential brittleness.
📚 Learning: 2025-08-21T19:19:44.538Z
Learnt from: jpan-box
PR: box/box-ui-elements#4237
File: src/api/ZipDownload.ts:6-8
Timestamp: 2025-08-21T19:19:44.538Z
Learning: For Box TypeScript SDK integrations, jpan-box prefers deep imports (e.g., from box-typescript-sdk-gen/lib/schemas/...generated.d.ts.js) over defining local types, citing that Box v2 APIs don't typically have breaking changes, making the deep import approach more viable despite potential brittleness.
Applied to files:
scripts/jest/jest.config.jssrc/api/ZipDownload.ts
🧬 Code graph analysis (2)
src/api/ZipDownload.ts (1)
src/features/collapsible-sidebar/CollapsibleSidebar.stories.js (1)
items(29-29)
src/api/APIFactory.js (1)
src/api/ZipDownload.ts (1)
ZipDownloadAPI(33-104)
🪛 Biome (2.1.2)
src/api/APIFactory.js
[error] 209-209: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 865-865: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ 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). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (2)
scripts/jest/jest.config.js (1)
29-30: Allowing Jest to transform jose is correct; unblocks ESM/CJS interopWhitelisting jose in transformIgnorePatterns is the right fix for the transform error originating from the SDK’s jose dependency. LGTM.
src/api/APIFactory.js (1)
209-210: Add zipDownloadAPI field is fine; keep Flow suppression localizedThe new field mirrors existing API members. No issues here.
There was a problem hiding this comment.
Actionable comments posted: 35
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
i18n/ko-KR.properties (1)
226-226: Correct typo in key prefix ‘commmon’ across codebaseThe lookup key
be.contentSidebar.activityFeed.commmon.showOriginalMessagecontains an extra “m” in “commmon”, which will prevent proper message resolution and fall back to missing strings. This occurs in both the message definition and all locale bundles. Please apply the following fixes:• src/elements/content-sidebar/activity-feed/common/activity-message/messages.js
– Change line 6 from
js id: 'be.contentSidebar.activityFeed.commmon.showOriginalMessage',
to
js id: 'be.contentSidebar.activityFeed.common.showOriginalMessage',• All locale files under i18n/*.properties (e.g., en-US, fr-FR, ko-KR, etc.), line 226 in each:
– Change
be.contentSidebar.activityFeed.commmon.showOriginalMessage = …
to
be.contentSidebar.activityFeed.common.showOriginalMessage = …You can verify and batch-update with:
# List all occurrences rg -l 'be\.contentSidebar\.activityFeed\.commmon\.' i18n src/elements/content-sidebar/activity-feed/common/activity-message/messages.js # Replace in-place rg -l 'be\.contentSidebar\.activityFeed\.commmon\.' i18n src/elements/content-sidebar/activity-feed/common/activity-message/messages.js \ | xargs sed -i 's/commmon\.showOriginalMessage/common.showOriginalMessage/g'i18n/zh-CN.properties (1)
226-226: Fix typo incommonprefix across message IDsThe key prefix is misspelled as
commmon(three m’s) in both the JavaScript definition and every locale properties file, causing lookup failures at runtime.Please update the following locations:
Message definition
File:src/elements/content-sidebar/activity-feed/common/activity-message/messages.jsexport default { - id: 'be.contentSidebar.activityFeed.commmon.showOriginalMessage', + id: 'be.contentSidebar.activityFeed.common.showOriginalMessage', defaultMessage: 'Show Original', };Locale resource files
In alli18n/*.properties(e.g.zh-CN.properties,en-US.properties, etc.) at line 226, replace:-be.contentSidebar.activityFeed.commmon.showOriginalMessage = <translated text> +be.contentSidebar.activityFeed.common.showOriginalMessage = <translated text>This change must be applied consistently across all affected files to restore correct message lookups.
🧹 Nitpick comments (23)
src/elements/content-sidebar/activity-feed/comment-form/CommentForm.scss (1)
38-41: Scope the selector to CommentForm to avoid unintended styling collisionsTighten the selector so this margin only applies when the toggle is inside CommentForm. Keeps styles predictable if the same class appears elsewhere.
Apply this diff:
- .bcs-CommentTimestamp-toggle { + .bcs-CommentForm .bcs-CommentTimestamp-toggle { margin-top: $bdl-grid-unit * 2.5; }i18n/en-AU.properties (1)
1829-1830: Nit: standardise comment style for ARIA.Elsewhere in this file we use “Aria label …” (no hyphen). Align this new comment accordingly for consistency.
-# Aria-label for the dropdown menu that shows actions for selected items +# Aria label for the dropdown menu that shows actions for selected items boxui.subHeader.bulkItemActionMenuAriaLabel = Bulk actionsi18n/en-CA.properties (1)
1829-1830: Nit: comment phrasing — remove hyphen in “Aria-label”.Maintain consistency with existing comments (“Aria label …”).
-# Aria-label for the dropdown menu that shows actions for selected items +# Aria label for the dropdown menu that shows actions for selected items boxui.subHeader.bulkItemActionMenuAriaLabel = Bulk actionsi18n/ko-KR.properties (3)
221-222: Untranslated label: localize the new timestamp toggle textThe value is still English. Provide a concise Korean translation to maintain locale consistency.
-# Label for toggle to add video timestamp to comment -be.contentSidebar.activityFeed.commentForm.commentTimestampLabel = Comment with timestamp +# Label for toggle to add video timestamp to comment +be.contentSidebar.activityFeed.commentForm.commentTimestampLabel = 코멘트에 타임스탬프 추가
1830-1831: ARIA label not localizedScreen-reader text should be localized like other ARIA strings.
-# Aria-label for the dropdown menu that shows actions for selected items -boxui.subHeader.bulkItemActionMenuAriaLabel = Bulk actions +# Aria-label for the dropdown menu that shows actions for selected items +boxui.subHeader.bulkItemActionMenuAriaLabel = 일괄 작업
1834-1836: Localize the time format hintThe copy still exposes the literal “HH:MM A”. Use a natural Korean pattern per the comment (“should be localized”).
-# Error message for empty time formats. "HH:MM A" should be localized. -boxui.timeInput.emptyTimeError = 필수 필드입니다. HH:MM A 형식으로 시간을 입력하십시오. -# Error message for invalid time formats. "HH:MM A" should be localized. -boxui.timeInput.invalidTimeError = 올바르지 않은 시간 형식입니다. HH:MM A 형식으로 시간을 입력하십시오. +# Error message for empty time formats. Example localized pattern. +boxui.timeInput.emptyTimeError = 필수 필드입니다. 오전/오후 시:분 형식으로 시간을 입력하십시오. +# Error message for invalid time formats. Example localized pattern. +boxui.timeInput.invalidTimeError = 올바르지 않은 시간 형식입니다. 오전/오후 시:분 형식으로 시간을 입력하십시오.src/components/form-elements/draft-js-mention-selector/__tests__/DraftJSMentionSelector.test.js (3)
29-34: Clarify test name and assert Toggle labelThe test name mentions a feature flag that isn’t a prop here; it’s gated by
timestampLabel. Also assert the label to tighten expectations.-test('should toggle the time stamp if isRequired and timestampedCommentsEnabled is true', () => { +test('should render Toggle when isRequired is true and timestampLabel is provided', () => { const wrapper = shallow( <DraftJSMentionSelector {...requiredProps} isRequired={true} timestampLabel={'Toggle Timestamp'} />, ); expect(wrapper.find('Toggle').length).toEqual(1); + expect(wrapper.find('Toggle').prop('label')).toEqual('Toggle Timestamp'); });
36-41: Rename for precisionName reflects implementation more clearly; behavior unchanged.
-test('should not toggle the time stamp if isRequired is false', () => { +test('should not render Toggle when isRequired is false', () => {
43-48: Fix casing in test title (timestampLabel)Avoid mixed casing to match the actual prop.
-test('should not toggle the time stamp if timeStampLabel is undefined', () => { +test('should not render Toggle when timestampLabel is undefined', () => {src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentForm.test.js (1)
149-156: Strengthen assertion: verify the actual timestamp label idGiven
intlFakereturnsmessage.id, we can assert the i18n id to prevent regressions.expect(wrapper.find('DraftJSMentionSelector').at(0).prop('timestampLabel')).toBeDefined(); +expect(wrapper.find('DraftJSMentionSelector').at(0).prop('timestampLabel')).toEqual( + 'be.contentSidebar.activityFeed.commentForm.commentTimestampLabel', +);i18n/zh-CN.properties (4)
221-222: Untranslated label: localize the new timestamp toggle textProvide a concise Simplified Chinese translation.
-# Label for toggle to add video timestamp to comment -be.contentSidebar.activityFeed.commentForm.commentTimestampLabel = Comment with timestamp +# Label for toggle to add video timestamp to comment +be.contentSidebar.activityFeed.commentForm.commentTimestampLabel = 带时间戳的评论
1829-1831: ARIA label not localizedLocalize the ARIA label for the bulk actions menu.
-# Aria-label for the dropdown menu that shows actions for selected items -boxui.subHeader.bulkItemActionMenuAriaLabel = Bulk actions +# Aria-label for the dropdown menu that shows actions for selected items +boxui.subHeader.bulkItemActionMenuAriaLabel = 批量操作
1834-1836: Polish time format copy“上午小时:分钟” reads awkwardly. Use a natural 12‑hour hint with 上午/下午 and 时:分.
-# Error message for empty time formats. "HH:MM A" should be localized. -boxui.timeInput.emptyTimeError = 必填字段。请以“上午小时:分钟”格式输入时间。 -# Error message for invalid time formats. "HH:MM A" should be localized. -boxui.timeInput.invalidTimeError = 时间格式无效。请以“上午小时:分钟”格式输入时间。 +# Error message for empty time formats. Localized 12‑hour example. +boxui.timeInput.emptyTimeError = 必填字段。请以“上午/下午 时:分”格式输入时间。 +# Error message for invalid time formats. Localized 12‑hour example. +boxui.timeInput.invalidTimeError = 时间格式无效。请以“上午/下午 时:分”格式输入时间。
2006-2006: Untranslated sentence (keep “Box Notes” in English as noted)Translate the sentence while preserving the product name per the comment.
-boxui.unifiedShare.sharedLinkPermissionsEditTooltip = This permission can only be changed in Box Notes +boxui.unifiedShare.sharedLinkPermissionsEditTooltip = 此权限只能在 Box Notes 中更改src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js (1)
112-119: Stabilize the test by avoiding displayName-based selectors and deep implementation couplingRelying on 'ForwardRef(withFeatureConsumer(CommentForm))' and digging into AnnotationActivityMenu internals via dive() is brittle. Prefer invoking the menu’s public onEdit handler and asserting on stable attributes/classes rendered by CommentForm (e.g., bcs-AnnotationActivity-editor) rather than the HOC-generated displayName.
Suggested change:
- wrapper.find(AnnotationActivityMenu).dive().simulate('click'); - wrapper.find(AnnotationActivityMenu).dive().find('MenuItem').simulate('click'); - expect(wrapper.exists('ForwardRef(withFeatureConsumer(CommentForm))')).toBe(true); + // Use the component’s public API instead of relying on internal MenuItem structure + wrapper.find(AnnotationActivityMenu).prop('onEdit')(); + expect(wrapper.exists('.bcs-AnnotationActivity-editor')).toBe(true); // Firing the onCancel prop will remove the CommentForm - wrapper.find('ForwardRef(withFeatureConsumer(CommentForm))').props().onCancel(); - expect(wrapper.exists('ForwardRef(withFeatureConsumer(CommentForm))')).toBe(false); + wrapper.findWhere(n => n.hasClass('bcs-AnnotationActivity-editor') && typeof n.prop('onCancel') === 'function') + .prop('onCancel')(); + expect(wrapper.exists('.bcs-AnnotationActivity-editor')).toBe(false);src/elements/content-sidebar/activity-feed/activity-feed/ActiveState.js (2)
142-154: onReplyUpdateHandler includes unused parameters (status, hasMention)Behavior looks unchanged; however, the unused parameters can confuse readers and may trip linters. Either prefix them with underscores or remove them if not required by the callback signature.
- ( - id: string, - text: string, - status?: FeedItemStatus, - hasMention?: boolean, - permissions: BoxCommentPermission, - onSuccess: ?Function, - onError: ?Function, - ) => { + ( + id: string, + text: string, + _status?: FeedItemStatus, + _hasMention?: boolean, + permissions: BoxCommentPermission, + onSuccess: ?Function, + onError: ?Function, + ) => { onReplyUpdate(id, parentId, text, permissions, onSuccess, onError); };
221-222: Duplicate prop: file is passed twice to BaseCommentfile is already present in commentAndAnnotationCommonProps. The explicit file={file} here is redundant; the later occurrence wins but this duplication can mislead readers.
- <BaseComment - {...commentAndAnnotationCommonProps} - file={file} + <BaseComment + {...commentAndAnnotationCommonProps}src/components/form-elements/draft-js-mention-selector/DraftJSMentionSelector.js (1)
295-297: Toggle rendered with a no-op onChange — clarify intent or wire it upIf the Toggle is meant to be interactive, we should expose controlled state and a callback. If it’s purely informational, consider rendering a non-interactive label instead of a Toggle to avoid UX confusion.
Option A (make it interactive; backward compatible):
- Add optional isTimestampOn and onTimestampToggle props.
- Control Toggle state and forward checked changes.
type Props = { @@ - timestampLabel?: string | null, + timestampLabel?: string | null, + isTimestampOn?: boolean, + onTimestampToggle?: (isOn: boolean) => void, @@ - {isRequired && timestampLabel && ( - <Toggle className="bcs-CommentTimestamp-toggle" label={timestampLabel} onChange={noop} /> - )} + {isRequired && timestampLabel && ( + <Toggle + className="bcs-CommentTimestamp-toggle" + label={timestampLabel} + name={`${name || 'DraftJSMentionSelector'}-timestamp`} + isOn={Boolean(this.props.isTimestampOn)} + onChange={event => + this.props.onTimestampToggle && this.props.onTimestampToggle(event.currentTarget.checked) + } + /> + )}Option B (non-interactive):
- Replace Toggle with a styled label element or InlineNotice.
Please confirm expected behavior; I can follow through with tests and wiring in CommentForm once decided.
src/elements/content-sidebar/activity-feed/comment/CreateReply.js (1)
52-66: Confirm whether replies should support timestamped comments; if yes, pass features throughIf timestamp labeling should also be available in reply forms, you’ll need to pass
featuresdown toCommentForm. If replies intentionally don’t support it, ignore this.Apply this diff if replies should support the feature:
@@ -import type { SelectorItems, BoxItem } from '../../../../common/types/core'; +import type { SelectorItems, BoxItem } from '../../../../common/types/core'; +import type { FeatureConfig } from '../../../common/feature-checking/flowTypes'; @@ type Props = { @@ isDisabled?: boolean, + features?: FeatureConfig, file?: BoxItem, @@ const CreateReply = ({ @@ onClick, - file, + file, + features, @@ <CommentForm @@ + features={features} file={file}src/elements/content-sidebar/activity-feed/comment-form/stories/CommentForm.stories.js (1)
98-101: Consider setting component to the unwrapped form for better Docs/Controls introspectionPointing
componenttoCommentFormUnwrappedoften yields clearer prop tables in Storybook Docs when HOCs are involved. Optional.-export default { +export default { title: 'Components/CommentForm', - component: CommentForm, + component: CommentFormUnwrapped,src/elements/content-sidebar/activity-feed/comment-form/CommentForm.js (3)
65-68: Remove stale default proptimestampedCommentsEnabledThis prop is no longer part of
CommentFormPropsnor referenced. Keeping it is confusing.class CommentForm extends React.Component<CommentFormProps, State> { static defaultProps = { isOpen: false, shouldFocusOnOpen: false, - timestampedCommentsEnabled: false, };
143-151: Normalize file extension and fix variable naming
- Normalize the extension to lowercase to handle cases like
MP4.- Use
isTimestampedCommentsEnabledfor readability.- const timestampCommentsConfig = getFeatureConfig(features, 'activityFeed.timestampedComments'); - - // Use feature config to determine if time stamped comments are enabled - const istimestampedCommentsEnabled = timestampCommentsConfig?.enabled === true; - const isVideo = FILE_EXTENSIONS.video.includes(file?.extension); - const allowVideoTimeStamps = isVideo && istimestampedCommentsEnabled; + const timestampCommentsConfig = getFeatureConfig(features, 'activityFeed.timestampedComments'); + const isTimestampedCommentsEnabled = timestampCommentsConfig?.enabled === true; + const ext = typeof file?.extension === 'string' ? file.extension.toLowerCase() : undefined; + const isVideo = !!ext && FILE_EXTENSIONS.video.includes(ext); + const allowVideoTimeStamps = isVideo && isTimestampedCommentsEnabled;
21-23: Flow suppression on TS import — narrow or document the reasonThe
$FlowFixMemakes sense due to importing from a TS module. Consider converting to$FlowExpectErrorwith a short reason or centralizing the check behind a tiny Flow-typed helper to avoid repeating suppressions elsewhere. Optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (41)
i18n/bn-IN.properties(2 hunks)i18n/da-DK.properties(2 hunks)i18n/de-DE.properties(2 hunks)i18n/en-AU.properties(2 hunks)i18n/en-CA.properties(2 hunks)i18n/en-GB.properties(2 hunks)i18n/en-US.properties(1 hunks)i18n/es-419.properties(2 hunks)i18n/es-ES.properties(2 hunks)i18n/fi-FI.properties(2 hunks)i18n/fr-CA.properties(2 hunks)i18n/fr-FR.properties(2 hunks)i18n/hi-IN.properties(2 hunks)i18n/it-IT.properties(2 hunks)i18n/ja-JP.properties(2 hunks)i18n/ko-KR.properties(2 hunks)i18n/nb-NO.properties(2 hunks)i18n/nl-NL.properties(2 hunks)i18n/pl-PL.properties(2 hunks)i18n/pt-BR.properties(2 hunks)i18n/ru-RU.properties(2 hunks)i18n/sv-SE.properties(2 hunks)i18n/tr-TR.properties(2 hunks)i18n/zh-CN.properties(2 hunks)i18n/zh-TW.properties(2 hunks)package.json(2 hunks)src/components/form-elements/draft-js-mention-selector/DraftJSMentionSelector.js(6 hunks)src/components/form-elements/draft-js-mention-selector/__tests__/DraftJSMentionSelector.test.js(3 hunks)src/elements/content-sidebar/activity-feed/activity-feed/ActiveState.js(6 hunks)src/elements/content-sidebar/activity-feed/activity-feed/ActivityFeed.js(2 hunks)src/elements/content-sidebar/activity-feed/activity-feed/__tests__/ActivityFeed.test.js(1 hunks)src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js(4 hunks)src/elements/content-sidebar/activity-feed/annotations/__tests__/AnnotationActivity.test.js(1 hunks)src/elements/content-sidebar/activity-feed/comment-form/CommentForm.js(5 hunks)src/elements/content-sidebar/activity-feed/comment-form/CommentForm.scss(1 hunks)src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentForm.test.js(4 hunks)src/elements/content-sidebar/activity-feed/comment-form/messages.js(1 hunks)src/elements/content-sidebar/activity-feed/comment-form/stories/CommentForm.stories.js(1 hunks)src/elements/content-sidebar/activity-feed/comment/BaseComment.js(4 hunks)src/elements/content-sidebar/activity-feed/comment/Comment.js(4 hunks)src/elements/content-sidebar/activity-feed/comment/CreateReply.js(4 hunks)
✅ Files skipped from review due to trivial changes (3)
- i18n/fi-FI.properties
- i18n/nb-NO.properties
- i18n/pl-PL.properties
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: jpan-box
PR: box/box-ui-elements#4237
File: src/api/ZipDownload.ts:6-8
Timestamp: 2025-08-21T19:19:44.538Z
Learning: For Box TypeScript SDK integrations, jpan-box prefers deep imports (e.g., from box-typescript-sdk-gen/lib/schemas/...generated.d.ts.js) over defining local types, citing that Box v2 APIs don't typically have breaking changes, making the deep import approach more viable despite potential brittleness.
Learnt from: jpan-box
PR: box/box-ui-elements#4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
Learning: In the box-ui-elements repository, there's a file-type-based pattern for internationalization: TypeScript files (.tsx) predominantly use the modern useIntl hook (41 vs 15 files), while JavaScript files (.js) predominantly use the legacy injectIntl HOC (64 vs 5 files). New TypeScript components should use useIntl, while existing JavaScript components typically continue using injectIntl for consistency.
📚 Learning: 2025-08-19T17:50:47.187Z
Learnt from: bfoxx1906
PR: box/box-ui-elements#4228
File: src/elements/content-sidebar/activity-feed/comment/Comment.js:147-148
Timestamp: 2025-08-19T17:50:47.187Z
Learning: In the box-ui-elements codebase, ActivityThreadReplyForm.js and AnnotationThreadCreate.js component files exist but are not actively used/imported anywhere, so they don't need updates for prop propagation.
Applied to files:
src/elements/content-sidebar/activity-feed/activity-feed/ActivityFeed.jssrc/elements/content-sidebar/activity-feed/comment/CreateReply.jssrc/elements/content-sidebar/activity-feed/activity-feed/ActiveState.jssrc/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js
📚 Learning: 2025-08-19T17:50:47.187Z
Learnt from: bfoxx1906
PR: box/box-ui-elements#4228
File: src/elements/content-sidebar/activity-feed/comment/Comment.js:147-148
Timestamp: 2025-08-19T17:50:47.187Z
Learning: ActivityThreadReplyForm and AnnotationThreadCreate components mentioned in the box-ui-elements codebase are not currently used and do not need to be updated for file prop propagation.
Applied to files:
src/elements/content-sidebar/activity-feed/comment/CreateReply.jssrc/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js
📚 Learning: 2025-08-19T17:44:59.234Z
Learnt from: bfoxx1906
PR: box/box-ui-elements#4228
File: src/elements/content-sidebar/activity-feed/activity-feed/ActiveState.js:185-186
Timestamp: 2025-08-19T17:44:59.234Z
Learning: In ActiveState.js, ActivityThread is only rendered when hasNewThreadedReplies is false. When hasNewThreadedReplies is true (newer threaded replies approach), BaseComment is rendered directly instead of ActivityThread. This means features using the newer threaded replies approach bypass ActivityThread entirely.
Applied to files:
src/elements/content-sidebar/activity-feed/activity-feed/ActiveState.js
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Applied to files:
src/elements/content-sidebar/activity-feed/activity-feed/ActiveState.js
📚 Learning: 2025-06-25T13:09:54.538Z
Learnt from: rafalmaksymiuk
PR: box/box-ui-elements#4160
File: src/elements/content-sidebar/SidebarToggle.js:21-27
Timestamp: 2025-06-25T13:09:54.538Z
Learning: The box-ui-elements project uses Flow for type annotations in JavaScript files, as indicated by flow directives in file headers. Type annotations like `: Props` are valid Flow syntax, not TypeScript syntax.
Applied to files:
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js
🧬 Code graph analysis (9)
src/components/form-elements/draft-js-mention-selector/DraftJSMentionSelector.js (1)
src/components/toggle/Toggle.js (1)
Toggle(36-58)
src/elements/content-sidebar/activity-feed/activity-feed/ActivityFeed.js (8)
src/elements/content-sidebar/activity-feed/activity-feed/__tests__/ActivityFeed.test.js (1)
file(60-75)src/elements/content-sidebar/__tests__/ActivitySidebar.test.js (1)
file(49-54)src/api/Feed.js (1)
file(2366-2366)src/api/__tests__/Feed.test.js (1)
file(349-364)src/elements/content-sidebar/__tests__/Sidebar.test.js (1)
file(28-33)src/elements/content-sidebar/activity-feed/activity-feed/stories/ActivityFeed.stories.js (1)
file(19-36)src/api/ThreadedComments.js (1)
file(110-110)src/api/Comments.js (3)
file(110-110)file(171-171)file(217-217)
src/elements/content-sidebar/activity-feed/comment-form/stories/CommentForm.stories.js (1)
src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentForm.test.js (1)
intlFake(12-14)
src/elements/content-sidebar/activity-feed/comment/CreateReply.js (1)
src/elements/content-sidebar/activity-feed/activity-feed/__tests__/ActivityFeed.test.js (1)
file(60-75)
src/elements/content-sidebar/activity-feed/activity-feed/ActiveState.js (7)
src/api/Feed.js (2)
permissions(196-196)file(2366-2366)src/elements/content-sidebar/__tests__/ActivitySidebar.test.js (1)
file(49-54)src/elements/content-sidebar/activity-feed/activity-feed/__tests__/ActivityFeed.test.js (1)
file(60-75)src/api/__tests__/Feed.test.js (1)
file(349-364)src/elements/content-sidebar/__tests__/Sidebar.test.js (1)
file(28-33)src/api/ThreadedComments.js (1)
file(110-110)src/api/Comments.js (3)
file(110-110)file(171-171)file(217-217)
src/elements/content-sidebar/activity-feed/comment/Comment.js (5)
src/elements/content-sidebar/__tests__/ActivitySidebar.test.js (1)
file(49-54)src/api/Feed.js (1)
file(2366-2366)src/api/__tests__/Feed.test.js (1)
file(349-364)src/api/ThreadedComments.js (1)
file(110-110)src/api/Comments.js (3)
file(110-110)file(171-171)file(217-217)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (6)
src/elements/content-sidebar/activity-feed/activity-feed/__tests__/ActivityFeed.test.js (1)
file(60-75)src/elements/content-sidebar/__tests__/ActivitySidebar.test.js (1)
file(49-54)src/api/Feed.js (1)
file(2366-2366)src/api/__tests__/Feed.test.js (1)
file(349-364)src/elements/content-sidebar/__tests__/Sidebar.test.js (1)
file(28-33)src/api/ThreadedComments.js (1)
file(110-110)
src/elements/content-sidebar/activity-feed/comment/BaseComment.js (3)
src/elements/content-sidebar/activity-feed/activity-feed/__tests__/ActivityFeed.test.js (1)
file(60-75)src/elements/content-sidebar/__tests__/ActivitySidebar.test.js (1)
file(49-54)src/elements/content-sidebar/activity-feed/activity-feed/stories/ActivityFeed.stories.js (1)
file(19-36)
src/elements/content-sidebar/activity-feed/comment-form/CommentForm.js (3)
src/elements/content-sidebar/activity-feed/comment-form/stories/CommentForm.stories.js (1)
features(59-65)src/elements/common/item/constants.ts (1)
FILE_EXTENSIONS(96-96)src/elements/content-sidebar/activity-feed/comment-form/messages.js (1)
messages(9-77)
🪛 Biome (2.1.2)
src/elements/content-sidebar/activity-feed/comment-form/stories/CommentForm.stories.js
[error] 5-5: 'import { type x ident }' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 24-24: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/activity-feed/comment/CreateReply.js
[error] 11-11: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/activity-feed/activity-feed/ActiveState.js
[error] 34-34: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 143-143: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 145-145: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 146-146: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 147-147: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 147-147: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 148-148: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 148-148: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 149-149: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 150-150: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 150-150: expected , but instead found ?
Remove ?
(parse)
[error] 150-150: expected , but instead found Function
Remove Function
(parse)
[error] 151-151: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 151-151: expected , but instead found ?
Remove ?
(parse)
[error] 151-151: expected , but instead found Function
Remove Function
(parse)
[error] 150-150: Do not shadow the global "Function" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 151-151: Do not shadow the global "Function" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 151-151: Duplicate parameter name.
The parameter overrides a preceding parameter by using the same name.
(lint/suspicious/noDuplicateParameters)
src/elements/content-sidebar/activity-feed/comment/Comment.js
[error] 27-27: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js
[error] 24-24: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/activity-feed/comment/BaseComment.js
[error] 26-26: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
src/elements/content-sidebar/activity-feed/comment-form/CommentForm.js
[error] 23-23: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 25-25: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 26-26: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 63-63: type arguments are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 74-74: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 74-74: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ 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). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (30)
src/elements/content-sidebar/activity-feed/activity-feed/__tests__/ActivityFeed.test.js (1)
442-442: I’ve added a script to inspect theCommentForm.jsexports and view its contents. This will confirm ifCommentFormBaseis actually exported or if we need to reference the wrapped component’s static property instead. Let me know the results, and I’ll adjust the recommendation accordingly.i18n/en-US.properties (1)
221-223: LGTM: adds timestamp toggle labelKey and copy read naturally and align with existing comment form strings.
i18n/en-AU.properties (1)
221-222: Timestamp Label Integration VerifiedAll cross-file usages of
commentTimestampLabelhave been confirmed:
src/elements/content-sidebar/activity-feed/comment-form/messages.jsdefines the ID with the matchingdefaultMessage(“Comment with timestamp”).CommentForm.jsconditionally formats and passestimestampLabelbased on the video feature flag.DraftJSMentionSelector.jsacceptstimestampLabeland renders the<Toggle>when provided.- Every locale file under
i18n/*.propertiesincludesbe.contentSidebar.activityFeed.commentForm.commentTimestampLabel, preventing fallback to en-US.i18n/en-CA.properties (1)
221-222: All checks passed for the timestamp toggle label• Message ID
be.contentSidebar.activityFeed.commentForm.commentTimestampLabelis defined inmessages.js.
• Consuming components (CommentForm,DraftJSMentionSelector) correctly receive and rendertimestampLabelwhen the feature flag is enabled, with accompanying tests in place.
• The key is present in all 26 locale files underi18n/*.properties, ensuring consistent screen-reader output across supported locales.No further action required.
i18n/en-GB.properties (2)
221-222: LGTM: timestamp toggle label added correctlyKey name and value read well for en-GB; no placeholders involved and the neighboring strings remain consistent.
1829-1830: LGTM: ARIA label for bulk actions is concise and accessibleClear, short label appropriate for a menu toggle. No ICU variables required.
src/elements/content-sidebar/activity-feed/comment-form/__tests__/CommentForm.test.js (5)
63-63: LGTM: requiredness toggles with isOpenProp wiring looks correct and matches UX.
Also applies to: 69-69
98-101: LGTM: placeholder omitted when seeded with tagged_messageAssertion matches intl stub behavior.
107-113: LGTM: placeholder falls back to i18n id via intl stubGood coverage of the empty state.
118-119: LGTM: disables mention trigger when provider is nullWiring to child prop looks correct.
130-131: LGTM: honors custom placeholderAssertion is precise and valuable.
src/elements/content-sidebar/activity-feed/activity-feed/ActivityFeed.js (1)
398-399: All downstream components correctly accept the newfileprop
I’ve verified that ActiveState, CommentForm, AnnotationActivity, BaseComment, and CreateReply each declarefile?: BoxItem(or requiredfile: BoxItemwhere appropriate) in their props and use it in rendering logic. Call sites now consistently passfile={file}. No further changes needed.src/elements/content-sidebar/activity-feed/comment/Comment.js (1)
27-28: I've initiated verification to confirm thatCommentFormindeed receives thefileprop. Once those results are in, I'll update the review accordingly.src/elements/content-sidebar/activity-feed/comment-form/messages.js (1)
72-76: New i18n message key for timestamp toggle looks good.Id, defaultMessage, and description are consistent with existing naming.
src/elements/content-sidebar/activity-feed/activity-feed/ActiveState.js (2)
34-34: Note on static analysis hints: Flow types are intentional hereThis file uses Flow (// @flow, import type ...). Biome’s TS-only warnings are false positives in this repo. No action needed unless you plan to migrate to TS.
181-186: Ignore incorrect component suggestions—file prop is already included and no<Comment>/<AnnotationActivity>in those branchesBoth “comment” and “annotation” cases use the shared
commentAndAnnotationCommonProps(which spreads infile) and render via<BaseComment>in their threaded-replies branch. In the non-threaded branch they render<ActivityThread>, not a<Comment>or<AnnotationActivity>, so no additionalfile={file}is needed.• The
commentAndAnnotationCommonPropsobject (lines 181–186) already includesfile, and<BaseComment {...commentAndAnnotationCommonProps}>applies it for both comments and annotations.
• The non-threaded paths render<ActivityThread …>, not a comment-specific component.
• There is no import or usage of<Comment>or<AnnotationActivity>inActiveState.js.You can safely disregard the proposed diffs for
<Comment>and<AnnotationActivity>.Likely an incorrect or invalid review comment.
src/components/form-elements/draft-js-mention-selector/DraftJSMentionSelector.js (3)
50-52: New prop timestampLabel — LGTMPublic API extension is straightforward and optional. Good gate via isRequired && timestampLabel before rendering.
153-157: Minor readability tweak — LGTMThe one-liner getPlainText().trim() usage is clear and equivalent. No functional change.
169-169: Minor simplification — LGTMDestructuring length from trimmed text reads cleanly.
src/elements/content-sidebar/activity-feed/annotations/AnnotationActivity.js (4)
34-35: Prop extension to accept file?: BoxItem — LGTMType import and optional prop are consistent with the rest of the feed code.
48-52: Destructuring file from props — LGTMNo behavior change here; simply preparing for the pass-through.
177-187: Propagating file to CommentForm — LGTMThis unblocks features (e.g., timestamp label) that depend on file context during edit.
Ensure ActiveState also passes file when rendering AnnotationActivity inside ActivityThread (non-threaded path); see suggestions in ActiveState.js for the missing pass-through.
24-24: Note on static analysis hints: Flow types are intentional hereThis repo uses Flow; Biome’s TS-only parse errors don’t apply.
src/elements/content-sidebar/activity-feed/comment/BaseComment.js (2)
40-41: BaseComment now accepts file?: BoxItem — LGTMType extension aligns with downstream CommentForm needs.
26-26: Note on static analysis hints: Flow types are intentional hereimport type is valid Flow syntax in this codebase. Safe to ignore TS-only warnings.
src/elements/content-sidebar/activity-feed/comment/CreateReply.js (2)
18-30: File context propagation into CommentForm — looks goodAdding
file?: BoxItemto Props, threading it through the component, and passing it toCommentFormaligns with the broader file-context propagation in this PR. No functional concerns.Also applies to: 41-42, 58-59
3-5: Keep using injectIntl in JS files (matches project conventions)Sticking with
injectIntlin JS files is consistent with the repo’s convention (JS → injectIntl, TS/TSX → useIntl). Good call.Also applies to: 76-76
src/elements/content-sidebar/activity-feed/comment-form/stories/CommentForm.stories.js (1)
58-96: Story exercises timestamped-comments path correctlyGood usage of
featuresand an MP4fileto exercise the toggle behavior. This will help validate the new UI branch.src/elements/content-sidebar/activity-feed/comment-form/CommentForm.js (2)
166-177: Passing timestampLabel into DraftJSMentionSelector is cleanly gatedNice, the toggle label is only provided when the feature is enabled and the file is a video. The prop boundary looks minimal and contained.
146-149: No action needed:FILE_EXTENSIONS.videokey is valid andtimestampLabelis supportedWe’ve confirmed in
src/elements/common/item/constants.tsthat:
FILE_EXTENSIONSis defined with a lowercasevideokey whose value is an array of extensions (e.g.,'3g2', '3gp')—soFILE_EXTENSIONS.video.includes(...)won’t throw at runtime.- There is no uppercase
VIDEOkey present.Additionally, in
DraftJSMentionSelector.js, thetimestampLabelprop is declared and used correctly, so enabling timestamp toggles will work as expected.
i18n/bn-IN.properties
Outdated
| # Aria-label for the dropdown menu that shows actions for selected items | ||
| boxui.subHeader.bulkItemActionMenuAriaLabel = Bulk actions |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
ARIA label left in English; Bengali translation needed.
Use phrasing consistent with “selected items” strings elsewhere in bn-IN.
Apply this diff:
-# Aria-label for the dropdown menu that shows actions for selected items
-boxui.subHeader.bulkItemActionMenuAriaLabel = Bulk actions
+# Aria-label for the dropdown menu that shows actions for selected items
+boxui.subHeader.bulkItemActionMenuAriaLabel = নির্বাচিত আইটেমগুলির জন্য পদক্ষেপRun to confirm no remaining English for these keys across locales:
🏁 Script executed:
#!/bin/bash
rg -nP 'be\.contentSidebar\.activityFeed\.commentForm\.commentTimestampLabel\s*=\s*Comment with timestamp' i18n
rg -nP 'boxui\.subHeader\.bulkItemActionMenuAriaLabel\s*=\s*Bulk actions' i18nLength of output: 5246
i18n: Translate bulk actions ARIA label to Bengali in bn-IN.properties
The ARIA label boxui.subHeader.bulkItemActionMenuAriaLabel is currently in English. Please update it to match existing “selected items” phrasing:
# Aria-label for the dropdown menu that shows actions for selected items
-boxui.subHeader.bulkItemActionMenuAriaLabel = Bulk actions
+boxui.subHeader.bulkItemActionMenuAriaLabel = নির্বাচিত আইটেমগুলির জন্য পদক্ষেপVerification also revealed:
- i18n/bn-IN.properties, line 222:
be.contentSidebar.activityFeed.commentForm.commentTimestampLabel = Comment with timestampremains in English; please add the appropriate Bengali translation. - In 23 other locale files (e.g., es-419.properties, zh-TW.properties, zh-CN.properties, tr-TR.properties, etc.),
boxui.subHeader.bulkItemActionMenuAriaLabel = Bulk actionsstill appears untranslated; coordinate to have this key localized across all remaining locales.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Aria-label for the dropdown menu that shows actions for selected items | |
| boxui.subHeader.bulkItemActionMenuAriaLabel = Bulk actions | |
| # Aria-label for the dropdown menu that shows actions for selected items | |
| boxui.subHeader.bulkItemActionMenuAriaLabel = নির্বাচিত আইটেমগুলির জন্য পদক্ষেপ |
🤖 Prompt for AI Agents
In i18n/bn-IN.properties around lines 1829-1830, replace the English ARIA label
value for boxui.subHeader.bulkItemActionMenuAriaLabel ("Bulk actions") with the
appropriate Bengali translation matching the existing "selected items" phrasing;
also update line 222
(be.contentSidebar.activityFeed.commentForm.commentTimestampLabel) to a Bengali
translation for "Comment with timestamp". After updating bn-IN, open a follow-up
to ensure the same key (boxui.subHeader.bulkItemActionMenuAriaLabel) is
localized across the remaining 23 locale files (e.g., es-419.properties,
zh-TW.properties, zh-CN.properties, tr-TR.properties) so each file contains the
translated string instead of the English phrase.
| # Label for toggle to add video timestamp to comment | ||
| be.contentSidebar.activityFeed.commentForm.commentTimestampLabel = Comment with timestamp | ||
| # Placeholder for comment input |
There was a problem hiding this comment.
English text in da-DK locale; translate the new comment timestamp toggle
Keep consistency with other “Tilføj …” labels in this section.
Apply this diff:
-be.contentSidebar.activityFeed.commentForm.commentTimestampLabel = Comment with timestamp
+be.contentSidebar.activityFeed.commentForm.commentTimestampLabel = Tilføj tidsstempel til kommentar📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Label for toggle to add video timestamp to comment | |
| be.contentSidebar.activityFeed.commentForm.commentTimestampLabel = Comment with timestamp | |
| # Placeholder for comment input | |
| # Label for toggle to add video timestamp to comment | |
| be.contentSidebar.activityFeed.commentForm.commentTimestampLabel = Tilføj tidsstempel til kommentar | |
| # Placeholder for comment input |
🤖 Prompt for AI Agents
In i18n/da-DK.properties around lines 221 to 223, the label value for
be.contentSidebar.activityFeed.commentForm.commentTimestampLabel is in English;
replace it with a Danish translation that follows the existing pattern of
“Tilføj …” labels (e.g., set the value to "Tilføj tidsstempel til kommentar")
and ensure the comment above remains accurate.
| # Label for toggle to add video timestamp to comment | ||
| be.contentSidebar.activityFeed.commentForm.commentTimestampLabel = Comment with timestamp | ||
| # Placeholder for comment input |
There was a problem hiding this comment.
Unlocalized English string in de-DE locale: translate "Comment with timestamp".
Provide a German label consistent with other action labels.
Apply this diff:
-# Label for toggle to add video timestamp to comment
-be.contentSidebar.activityFeed.commentForm.commentTimestampLabel = Comment with timestamp
+# Label for toggle to add video timestamp to comment
+be.contentSidebar.activityFeed.commentForm.commentTimestampLabel = Mit Zeitstempel kommentieren📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Label for toggle to add video timestamp to comment | |
| be.contentSidebar.activityFeed.commentForm.commentTimestampLabel = Comment with timestamp | |
| # Placeholder for comment input | |
| # Label for toggle to add video timestamp to comment | |
| be.contentSidebar.activityFeed.commentForm.commentTimestampLabel = Mit Zeitstempel kommentieren | |
| # Placeholder for comment input |
🤖 Prompt for AI Agents
In i18n/de-DE.properties around lines 221 to 223 the value for
be.contentSidebar.activityFeed.commentForm.commentTimestampLabel is still in
English; replace "Comment with timestamp" with a German label consistent with
other action labels, e.g. "Kommentar mit Zeitstempel" (or use the imperative
form "Mit Zeitstempel kommentieren" if your locale uses verbs for similar
toggles).
i18n/zh-TW.properties
Outdated
| boxui.subHeader.bulkItemActionMenuAriaLabel = Bulk actions | ||
| # Text for metadata button that will open the metadata side panel |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Untranslated ARIA label: localize “Bulk actions”
ARIA labels should be localized for assistive technologies. This remains in English.
Apply this diff:
-# Aria-label for the dropdown menu that shows actions for selected items
-boxui.subHeader.bulkItemActionMenuAriaLabel = Bulk actions
+# Aria-label for the dropdown menu that shows actions for selected items
+boxui.subHeader.bulkItemActionMenuAriaLabel = 批次動作Optional verification script to catch any remaining English fallbacks of these keys outside en-US:
🏁 Script executed:
#!/bin/bash
# Find locales (excluding en-US) where the new keys are still English
rg -nP -g 'i18n/*.properties' -g '!i18n/en-US.properties' \
$'be\\.contentSidebar\\.activityFeed\\.commentForm\\.commentTimestampLabel\\s*=\\s*Comment with timestamp|boxui\\.subHeader\\.bulkItemActionMenuAriaLabel\\s*=\\s*Bulk actions'Length of output: 5074
Localize “Bulk actions” ARIA label across all non-en-US locales
Verification shows that boxui.subHeader.bulkItemActionMenuAriaLabel (and the related key be.contentSidebar.activityFeed.commentForm.commentTimestampLabel) remain in English in every non-en-US properties file:
• i18n/es-ES.properties
• i18n/pl-PL.properties
• i18n/zh-CN.properties
• i18n/es-419.properties
• i18n/zh-TW.properties
• i18n/sv-SE.properties
• i18n/nl-NL.properties
• i18n/pt-BR.properties
• i18n/tr-TR.properties
• i18n/nb-NO.properties
• i18n/ja-JP.properties
• i18n/ko-KR.properties
• i18n/fr-FR.properties
• i18n/fr-CA.properties
• i18n/fi-FI.properties
• i18n/hi-IN.properties
• i18n/en-GB.properties
• i18n/de-DE.properties
• i18n/en-AU.properties
• i18n/da-DK.properties
• i18n/ru-RU.properties
• i18n/en-CA.properties
• i18n/bn-IN.properties
• i18n/it-IT.properties
Please update each file with the appropriate localized strings. For example, in zh-TW:
-# Aria-label for the dropdown menu that shows actions for selected items
-boxui.subHeader.bulkItemActionMenuAriaLabel = Bulk actions
+# Aria-label for the dropdown menu that shows actions for selected items
+boxui.subHeader.bulkItemActionMenuAriaLabel = 批次動作Additionally, ensure the be.contentSidebar.activityFeed.commentForm.commentTimestampLabel key (“Comment with timestamp”) is also localized across the same set of locale files.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| boxui.subHeader.bulkItemActionMenuAriaLabel = Bulk actions | |
| # Text for metadata button that will open the metadata side panel | |
| # Aria-label for the dropdown menu that shows actions for selected items | |
| boxui.subHeader.bulkItemActionMenuAriaLabel = 批次動作 | |
| # Text for metadata button that will open the metadata side panel |
🤖 Prompt for AI Agents
In i18n/zh-TW.properties around lines 1830-1831, the value for
boxui.subHeader.bulkItemActionMenuAriaLabel is left in English ("Bulk actions");
also ensure be.contentSidebar.activityFeed.commentForm.commentTimestampLabel
("Comment with timestamp") is localized; update these two keys in zh-TW and the
same set of non-en-US locale files (listed in the review) with the correct
localized strings (replace the English values with proper translations),
preserving the existing key formatting and escaping rules, and add the keys if
missing.
| approverSelectorContacts?: SelectorItems<>, | ||
| currentFileVersionId: string, | ||
| currentUser?: User, | ||
| file: BoxItem, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New required prop "file: BoxItem" — verify all call sites are updated
Making file required is fine, but it’s widely threaded. Please verify all <ActiveState ...> usages pass file to avoid runtime/Flow breakages.
Run this quick check:
🏁 Script executed:
#!/bin/bash
# Find ActiveState JSX without a file prop
rg -nP '\<ActiveState\b(?![^>]*\bfile=)' -C2 --type=js --type=jsx --type=ts --type=tsxLength of output: 116
I’ve adjusted the ripgrep invocation to use glob patterns instead of unsupported type flags. Please re-run this check to locate any <ActiveState> usages missing the new file prop:
🏁 Script executed:
#!/bin/bash
# Find ActiveState JSX without a file prop across .js/.jsx/.ts/.tsx files
rg -nP '<ActiveState\b(?![^>]*\bfile=)' -C2 -g '*.js' -g '*.jsx' -g '*.ts' -g '*.tsx'Length of output: 1353
Critical: file prop missing on <ActiveState> call sites
Please update every usage of <ActiveState> to include the newly required file: BoxItem prop to prevent runtime and Flow errors.
Locations needing fixes:
- src/elements/content-sidebar/activity-feed/activity-feed/ActivityFeed.js (around line 391)
Current:Suggested:<ActiveState {...activityFeedError} activeFeedItem={activeFeedItem} />
<ActiveState {...activityFeedError} activeFeedItem={activeFeedItem} + file={activeFeedItem.file} /> - src/elements/content-sidebar/activity-feed/activity-feed/tests/ActiveState.test.js (around line 125)
Current:Suggested (add a mock<ActiveState items={[annotation, comment, fileVersion, taskWithAssignment, appActivity]} currentUser={currentUser} />
fileprop, e.g.file={fileVersion}or a dedicated mock):<ActiveState items={[annotation, comment, fileVersion, taskWithAssignment, appActivity]} currentUser={currentUser} + file={fileVersion} />
Ensure any other <ActiveState> instances also pass file.
🤖 Prompt for AI Agents
In src/elements/content-sidebar/activity-feed/activity-feed/ActiveState.js
around line 52 the component now requires a file: BoxItem prop but call sites
are missing it; update every usage (notably
src/elements/content-sidebar/activity-feed/activity-feed/ActivityFeed.js ~line
391 and
src/elements/content-sidebar/activity-feed/activity-feed/__tests__/ActiveState.test.js
~line 125) to pass a BoxItem-valued file prop (in ActivityFeed pass the relevant
file object available in scope; in the test add a mock file such as
file={fileVersion} or create a dedicated mock BoxItem and pass file={mockFile});
ensure Flow types/imports remain satisfied and update any other ActiveState
usages similarly.
| @@ -0,0 +1,173 @@ | |||
| // @flow | |||
| import * as React from 'react'; | |||
| import { action } from 'storybook/actions'; | |||
There was a problem hiding this comment.
Incorrect Storybook actions import path
This will fail at runtime. Use the official addon import.
Apply this diff:
-import { action } from 'storybook/actions';
+import { action } from '@storybook/addon-actions';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { action } from 'storybook/actions'; | |
| import { action } from '@storybook/addon-actions'; |
🤖 Prompt for AI Agents
In
src/elements/content-sidebar/activity-feed/comment-form/stories/CommentForm.stories.js
around line 3, the Storybook actions import uses the incorrect path
'storybook/actions' which will fail at runtime; change the import to the
official addon package by importing action from '@storybook/addon-actions'
instead, updating the import statement accordingly so Storybook can resolve the
addon.
| file={file} | ||
| getAvatarUrl={getAvatarUrl} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Propagating file to CommentForm — LGTM, but propagate to replies too
Top-level edits now have file context. However, replies rendered via Replies don’t receive file, so editing a reply won’t pass file to its nested CommentForm. Recommend threading file through Replies and into its BaseComment children.
Proposed changes:
- Pass file into Replies from BaseComment:
@@
- {hasReplies && (
- <Replies
+ {hasReplies && (
+ <Replies
+ file={file}
{...commentProps}- Extend RepliesProps with file and forward it to child BaseComment:
type RepliesProps = {
currentUser?: User,
+ file?: BoxItem,
@@
export const Replies = ({
currentUser,
+ file,
@@
return (
<li key={`${type}${id}`}>
<BaseComment
{...reply}
+ file={file}
currentUser={currentUser}🏁 Script executed:
#!/bin/bash
# Locate the Replies component file
REPLIES_FILE=$(fd -t f -e js --max-depth 5 'Replies.js' src/elements/content-sidebar/activity-feed/comment)
echo "Replies file: $REPLIES_FILE"
# Show the beginning of the file to inspect props and type definition
sed -n '1,200p' "$REPLIES_FILE"
# Check the type definition for RepliesProps
rg -n "type RepliesProps" -C5 "$REPLIES_FILE"
# Search for any existing usage of 'file' within Replies
rg -n "file" -C3 "$REPLIES_FILE"Length of output: 347
🏁 Script executed:
#!/bin/bash
# Search for the Replies component under the activity-feed/comment directory
echo "Searching for Replies files in activity-feed/comment..."
fd -t f -e js,jsx,ts,tsx -I 'Replies*' src/elements/content-sidebar/activity-feed/comment
# If none found, search the entire repo
echo "Searching for Replies files in src/..."
fd -t f -e js,jsx,ts,tsx -I 'Replies*' src
# Once found, inspect the file
REPLIES_PATH=$(fd -t f -e js,jsx,ts,tsx -I 'Replies*' src/elements/content-sidebar/activity-feed/comment | head -n1)
if [ -n "$REPLIES_PATH" ]; then
echo "Found Replies at $REPLIES_PATH"
echo "First 200 lines of the file:"
sed -n '1,200p' "$REPLIES_PATH"
echo "Looking for type or prop definitions:"
rg -nP "(type|interface)\s+RepliesProps" -C5 "$REPLIES_PATH" || echo "No explicit type definition"
rg -nP "export\s+(const|function)\s+Replies" -C3 "$REPLIES_PATH"
echo "Checking for any existing file prop usage:"
rg -n "file" -C3 "$REPLIES_PATH"
else
echo "Replies component not found in activity-feed/comment"
fiLength of output: 596
🏁 Script executed:
#!/bin/bash
echo "Searching for definitions of the Replies component in src directory…"
rg -nP "export\s+(const|function)\s+Replies" -C1 src
rg -nP "function\s+Replies\s*\(" -C1 src
rg -nP "class\s+Replies\s*" -C1 srcLength of output: 895
🏁 Script executed:
#!/bin/bash
# Display the portion of BaseComment.js where Replies is defined and used
sed -n '200,350p' src/elements/content-sidebar/activity-feed/comment/BaseComment.jsLength of output: 6013
Propagate the file prop through Replies to nested BaseComment children
The file prop is passed to the top-level comment but dropped in replies (since RepliesProps doesn’t include file and the component signature doesn’t destructure it). As a result, editing a reply won’t carry the correct file context.
Please update:
- In the root render of
BaseComment(src/elements/content-sidebar/activity-feed/comment/BaseComment.js), passfileintoReplies:@@ BaseComment.js {hasReplies && ( - <Replies - {...commentProps} + <Replies + file={file} + {...commentProps} isParentPending={isPending} isRepliesLoading={isRepliesLoading} onCommentEdit={onCommentEdit} onHideReplies={onHideReplies} onReplyCreate={onReplyCreate} onReplyDelete={onReplyDelete} onReplySelect={onSelect} onShowReplies={onShowReplies} replies={replies} repliesTotalCount={repliesTotalCount} /> )} - Extend the
RepliesPropstype and function signature to includefile?: BoxItem:@@ BaseComment.js type RepliesProps = { currentUser?: User, + file?: BoxItem, getAvatarUrl: GetAvatarUrlCallback, getMentionWithQuery?: (searchStr: string) => void, getUserProfileUrl?: GetProfileUrlCallback, … }; export const Replies = ({ currentUser, + file, getAvatarUrl, getMentionWithQuery, getUserProfileUrl, isParentPending = false, isRepliesLoading = false, … }: RepliesProps) => { … return ( … <ol className="bcs-Replies-list"> {replies.map(reply => ( <li key={`${reply.type}${reply.id}`}> <BaseComment + file={file} {...reply} currentUser={currentUser} getAvatarUrl={getAvatarUrl} getMentionWithQuery={getMentionWithQuery} getUserProfileUrl={getUserProfileUrl} isPending={isParentPending || reply.isPending} mentionSelectorContacts={mentionSelectorContacts} onCommentEdit={onCommentEdit} onSelect={onReplySelect} onDelete={onReplyDelete} permissions={getReplyPermissions(reply)} translations={translations} /> </li> ))} </ol> … ); };
This will ensure replies—and their nested CommentForm—receive the same file context as top-level comments.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| file={file} | |
| getAvatarUrl={getAvatarUrl} | |
| --- a/src/elements/content-sidebar/activity-feed/comment/BaseComment.js | |
| @@ (around the root render of Replies) | |
| - {hasReplies && ( | |
| - <Replies | |
| {hasReplies && ( | |
| <Replies | |
| file={file} | |
| {...commentProps} | |
| isParentPending={isPending} | |
| isRepliesLoading={isRepliesLoading} | |
| onCommentEdit={onCommentEdit} | |
| onHideReplies={onHideReplies} | |
| onReplyCreate={onReplyCreate} | |
| onReplyDelete={onReplyDelete} | |
| onReplySelect={onSelect} | |
| onShowReplies={onShowReplies} | |
| replies={replies} | |
| repliesTotalCount={repliesTotalCount} | |
| /> | |
| )} | |
| @@ (around the RepliesProps type and component signature) | |
| type RepliesProps = { | |
| currentUser?: User, | |
| file?: BoxItem, | |
| getAvatarUrl: GetAvatarUrlCallback, | |
| getMentionWithQuery?: (searchStr: string) => void, | |
| getUserProfileUrl?: GetProfileUrlCallback, | |
| … | |
| }; | |
| export const Replies = ({ | |
| currentUser, | |
| file, | |
| getAvatarUrl, | |
| getMentionWithQuery, | |
| getUserProfileUrl, | |
| isParentPending = false, | |
| isRepliesLoading = false, | |
| … | |
| }: RepliesProps) => { | |
| … | |
| return ( | |
| … | |
| <ol className="bcs-Replies-list"> | |
| {replies.map(reply => ( | |
| <li key={`${reply.type}${reply.id}`}> | |
| <BaseComment | |
| file={file} | |
| {...reply} | |
| currentUser={currentUser} | |
| getAvatarUrl={getAvatarUrl} | |
| getMentionWithQuery={getMentionWithQuery} | |
| getUserProfileUrl={getUserProfileUrl} | |
| isPending={isParentPending || reply.isPending} | |
| mentionSelectorContacts={mentionSelectorContacts} | |
| onCommentEdit={onCommentEdit} | |
| onSelect={onReplySelect} | |
| onDelete={onReplyDelete} | |
| permissions={getReplyPermissions(reply)} | |
| translations={translations} | |
| /> | |
| </li> | |
| ))} | |
| </ol> | |
| … | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In src/elements/content-sidebar/activity-feed/comment/BaseComment.js around
lines 200-201, the file prop is passed to the top-level comment but not
propagated into Replies; update the root render to pass file={file} into the
Replies component, then extend the RepliesProps type to include file?: BoxItem
and update the Replies function signature to destructure file (i.e., function
Replies({ ..., file }: RepliesProps)), so that Replies forwards file down to its
nested BaseComment/CommentForm children ensuring reply edits have the correct
file context.
902d445 to
65c655c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/api/APIFactory.js (1)
209-210: Add destroy handling for zipDownloadAPI to prevent leaks.Symmetry with other APIs is missing; the instance sticks around after destroy(). I pity the leak, fool!
Apply near other cleanup blocks in destroy():
if (this.intelligenceAPI) { this.intelligenceAPI.destroy(); delete this.intelligenceAPI; } + + if (this.zipDownloadAPI) { + if (typeof this.zipDownloadAPI.destroy === 'function') { + this.zipDownloadAPI.destroy(); + } + delete this.zipDownloadAPI; + }Also consider a no-op destroy() in ZipDownload.ts for consistency:
public destroy(): void { // no-op }Want me to open a follow-up PR wiring this in both places?
🧹 Nitpick comments (1)
src/api/APIFactory.js (1)
39-41: Don’t hide type issues with a blanket $FlowFixMe, fool.Add a tiny Flow libdef stub for ZipDownload to keep type-safety and avoid masking real errors. I pity the fool who lets types slide!
Example stub (adjust path to match repo layout):
// flow-typed/zipdownload.js declare module 'src/api/ZipDownload' { declare export default class ZipDownloadAPI {} }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP (Model Context Protocol) integration is disabled
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
package.json(4 hunks)scripts/jest/jest-setup.ts(1 hunks)scripts/jest/jest.config.js(1 hunks)src/api/APIFactory.js(3 hunks)src/api/ZipDownload.ts(1 hunks)src/api/__tests__/ZipDownload.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/api/tests/ZipDownload.test.ts
- scripts/jest/jest.config.js
- src/api/ZipDownload.ts
- scripts/jest/jest-setup.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-21T19:19:44.584Z
Learnt from: jpan-box
PR: box/box-ui-elements#4237
File: src/api/ZipDownload.ts:6-8
Timestamp: 2025-08-21T19:19:44.584Z
Learning: For Box TypeScript SDK integrations, jpan-box prefers deep imports (e.g., from box-typescript-sdk-gen/lib/schemas/...generated.d.ts.js) over defining local types, citing that Box v2 APIs don't typically have breaking changes, making the deep import approach more viable despite potential brittleness.
Applied to files:
package.json
📚 Learning: 2025-06-17T15:19:52.028Z
Learnt from: tjuanitas
PR: box/box-ui-elements#4126
File: scripts/buildLocaleBundles.js:39-40
Timestamp: 2025-06-17T15:19:52.028Z
Learning: In scripts/buildLocaleBundles.js, the user intentionally wants to kill ALL running `build:prod:dist` processes on error (not just the ones spawned by the script) for comprehensive cleanup. They've confirmed that tracking individual PIDs from execSync is not feasible and their current approach serves their intended purpose.
Applied to files:
package.json
📚 Learning: 2025-08-12T18:04:17.698Z
Learnt from: tjuanitas
PR: box/box-ui-elements#4224
File: package.json:296-297
Timestamp: 2025-08-12T18:04:17.698Z
Learning: In the box-ui-elements project, the team is comfortable with raising peerDependency minimum versions when upgrading blueprint-web packages, even if it's a breaking change for consumers.
Applied to files:
package.json
🧬 Code graph analysis (1)
src/api/APIFactory.js (1)
src/api/ZipDownload.ts (1)
ZipDownloadAPI(33-104)
🪛 Biome (2.1.2)
src/api/APIFactory.js
[error] 209-209: return types can only be used in TypeScript files
remove this type annotation
(parse)
[error] 865-865: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ 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). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (3)
package.json (3)
61-61: LGTM: add --progress to prod build.Nice QoL improvement for CI logs. Respect, fool.
309-309: Keep dev and peer versions of @box/metadata-view aligned.They both read ^0.41.2. If the intent was to pin or downgrade, confirm downstreams won’t break. Speak now, fool.
142-142: No downgrade detected—remove version check
Dev and peer deps remain at ^0.41.2 (Yarn resolves to patch 0.41.3), so there’s no downgrade. Pity the fool who thinks otherwise.Likely an incorrect or invalid review comment.
| "dependencies": { | ||
| "box-typescript-sdk-gen": "^1.17.1" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move box-typescript-sdk-gen to peerDeps (and devDeps) to avoid bundling the whole SDK.
This is a library; shipping the SDK in dependencies risks duplicate installs and bigger bundles for consumers. Make it a peer, keep a devDep for local builds. Don’t weigh folks down, fool.
Apply this diff:
- "dependencies": {
- "box-typescript-sdk-gen": "^1.17.1"
- },
+ "dependencies": {},
"devDependencies": {
+ "box-typescript-sdk-gen": "^1.17.1",And add to peerDependencies:
"peerDependencies": {
+ "box-typescript-sdk-gen": ">=1.17.1",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "dependencies": { | |
| "box-typescript-sdk-gen": "^1.17.1" | |
| }, | |
| - "dependencies": { | |
| - "box-typescript-sdk-gen": "^1.17.1" | |
| "dependencies": {}, |
| getZipDownloadAPI(): ZipDownloadAPI { | ||
| this.zipDownloadAPI = new ZipDownloadAPI(this.options); | ||
| return this.zipDownloadAPI; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Guard against non-string tokens; require a concrete token for ZipDownload.
ZipDownloadAPI expects a string token, but APIFactory forwards options where token can be string|function. That’ll blow up at runtime, fool.
Apply this diff to enforce a concrete token and optionally accept a file name:
- getZipDownloadAPI(): ZipDownloadAPI {
- this.zipDownloadAPI = new ZipDownloadAPI(this.options);
- return this.zipDownloadAPI;
- }
+ getZipDownloadAPI(opts?: { downloadFileName?: string }): ZipDownloadAPI {
+ const { token } = this.options;
+ if (typeof token !== 'string') {
+ throw new Error('ZipDownloadAPI requires a concrete string token. Resolve function/async tokens before calling getZipDownloadAPI.');
+ }
+ this.zipDownloadAPI = new ZipDownloadAPI({
+ token,
+ downloadFileName: opts?.downloadFileName,
+ });
+ return this.zipDownloadAPI;
+ }If you prefer passing the token explicitly (to mirror past patterns), I can switch the signature to getZipDownloadAPI(token: string, opts?)—say the word.
Guard getZipDownloadAPI against non-string tokens, fool—throw if token isn’t a string!
- Throw an Error in getZipDownloadAPI when this.options.token isn’t a string before new ZipDownloadAPI(...)
- Add an optional
optsparameter fordownloadFileNameand pass it into the ZipDownloadAPI constructor
🧰 Tools
🪛 Biome (2.1.2)
[error] 865-865: return type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
🤖 Prompt for AI Agents
In src/api/APIFactory.js around lines 865 to 868, getZipDownloadAPI must
validate this.options.token and accept an optional opts parameter for
downloadFileName: before instantiating ZipDownloadAPI, check that typeof
this.options.token === 'string' and if not throw a new Error with a clear
message; change the method signature to accept an optional opts parameter (e.g.,
opts = {}) and pass that opts into the new ZipDownloadAPI(this.options, opts)
constructor so the downloadFileName option is forwarded.
3 participants
Added the API logic for zipdownload via the box-typescript-sdk package
Summary by CodeRabbit