feat(UI): OpenGraph extension upload dialog BED-6759#2349
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a single-file JSON SchemaUploadDialog (component, hook, tests), FileDrop customization (multiple/icon), upload progress utility, client API method for PUT /api/v2/extensions, and a quick-upload enablement hook that gates global drag-to-upload by path/permission; Content.tsx updated to use the new gate. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as FileDrop / SchemaUploadDialog
participant Handlers as useSchemaUploadHandlers
participant Client as BHEAPIClient
participant Backend as Server
User->>UI: Drag JSON file or click "Upload"
UI->>Handlers: handleFileDrop(file)
Handlers->>UI: set file (READY)
User->>UI: Click Upload
UI->>Handlers: handleUpload()
Handlers->>Client: uploadSchemaFile(file, { onUploadProgress })
Client->>Backend: PUT /api/v2/extensions
Backend-->>Client: response + progress events
Client-->>Handlers: onUploadProgress(event)
Handlers->>UI: update uploadProgress / status (UPLOADING -> DONE/FAILURE)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
maffkipp
added
the
user interface
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@packages/javascript/bh-shared-ui/src/components/SchemaUploadDialog/SchemaUploadDialog.test.tsx`:
- Around line 138-145: The failure test uses synchronous queries after the async
upload (inside withoutErrorLogging -> user.click) which can be flaky; change the
assertions to await the async queries instead (use await
screen.findByText('Failed to Upload') and await screen.findByRole('button', {
name: 'Retry upload' })) so the test waits for the error UI to appear, and keep
verifying addNotificationMock was called after those awaits.
In
`@packages/javascript/bh-shared-ui/src/components/SchemaUploadDialog/SchemaUploadDialog.tsx`:
- Line 34: The SchemaUploadDialog component is defined but not exported from the
package barrel; add an index.ts next to the SchemaUploadDialog component that
re-exports it, and update the package components barrel to follow the
established pattern by adding both export * from './SchemaUploadDialog' and
export { default as SchemaUploadDialog } from './SchemaUploadDialog' so parent
projects can import the SchemaUploadDialog symbol.
In
`@packages/javascript/bh-shared-ui/src/components/SchemaUploadDialog/useSchemaUploadHandlers.ts`:
- Around line 55-77: The current upload call in useSchemaUploadHandlers uses
file.file.type directly for the Content-Type header, which can be an empty
string; update the headers passed into uploadSchemaFile.mutate to use a safe
fallback (e.g., default to 'application/octet-stream' or derive from the
filename) when file.file.type is falsy so the request never sends an empty
Content-Type; modify the options.headers assignment in the
uploadSchemaFile.mutate call (where file.file.type is referenced) to compute
contentType = file.file.type || 'application/octet-stream' (or similar) before
calling setUploadProgress/calculateUploadProgress and the onError/onSuccess
handlers that setNewFileStatus and FileStatus.
🧹 Nitpick comments (5)
packages/javascript/js-client-library/src/client.ts (1)
964-966: Consider renaming thejsonparameter todataorfile.The parameter name
jsonis misleading since the caller passes aFileobject (fromuseSchemaUploadHandlers.ts, Line 108). Other upload methods in this file (e.g.,uploadFileToIngestJobat Line 939) also usejsonfor the payload parameter, so this is at least internally consistent, but it's still a source of confusion.♻️ Optional: clarify parameter name
- uploadSchemaFile = (json: any, options?: RequestOptions) => - this.baseClient.put('/api/v2/extensions', json, options); + uploadSchemaFile = (data: any, options?: RequestOptions) => + this.baseClient.put('/api/v2/extensions', data, options);packages/javascript/bh-shared-ui/src/components/SchemaUploadDialog/useSchemaUploadHandlers.ts (1)
100-109:fileproperty inUploadSchemaParamsshould not be optional.The
filefield is marked optional (file?: File), which meansuploadSchemaFilecan be called withundefined. WhilehandleUploadguards against this at the hook level, the mutation's type contract should reflect that a file is always required when calling the mutation. SinceuseUploadSchemaFileis module-private, this is low risk, but tightening the type avoids future misuse.♻️ Tighten the type
interface UploadSchemaParams { - file?: File; + file: File; options?: RequestOptions; }packages/javascript/bh-shared-ui/src/components/FileDrop/FileDrop.tsx (2)
39-43: Drag-and-drop does not enforcemultiple=false— by design, but worth documenting.The HTML5 drag-and-drop API does not respect the
<input multiple>attribute, so whenmultiple={false}, users can still drop multiple files via drag-and-drop. The enforcement happens in the consumer (e.g.,useSchemaUploadHandlers.handleFileDropchecksfiles.length === 1). This works, but a brief inline comment would help future maintainers understand why the single-file check isn't here.
82-86: Single-file message hardcodes "JSON file".The text
'Click here or drag and drop to upload a JSON file'may not be accurate ifFileDropis reused for non-JSON single-file uploads in the future. For now this is fine since it's only used for schema upload, but consider accepting the message as a prop if reuse is expected.packages/javascript/bh-shared-ui/src/components/SchemaUploadDialog/SchemaUploadDialog.test.tsx (1)
25-26: ClearaddNotificationMockbetween tests to avoid cross-test contamination.
addNotificationMockaccumulates calls across all tests since it's never reset. If a preceding test inadvertently triggers a notification, the assertion on line 144 could pass even if the error path didn't actually call it. Add amockClear()in the existingafterEach.Proposed fix
-afterEach(() => server.resetHandlers()); +afterEach(() => { + server.resetHandlers(); + addNotificationMock.mockClear(); +});Also applies to: 72-72
coderabbitai
Bot
added
the
api
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@packages/javascript/bh-shared-ui/src/components/SchemaUploadDialog/SchemaUploadDialog.test.tsx`:
- Around line 25-35: The module-scoped mock addNotificationMock is never cleared
between tests, causing accumulated call counts and false positives; fix by
adding a test teardown that clears or resets the mock (e.g., call
addNotificationMock.mockClear() or vi.clearAllMocks()) in an afterEach/afterAll
hook in SchemaUploadDialog.test.tsx so each test starts with a fresh
addNotificationMock used by the mocked useNotifications provider.
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/components/SchemaUploadDialog/useSchemaUploadHandlers.ts (1)
100-103: Consider makingfilerequired inUploadSchemaParams.
fileis declared optional, butapiClient.uploadSchemaFile(undefined, options)would be an invalid call. SincehandleUploadalways supplies it, making the field required would catch accidental misuse at compile time.Suggested fix
interface UploadSchemaParams { - file?: File; + file: File; options?: RequestOptions; }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@packages/javascript/bh-shared-ui/src/components/SchemaUploadDialog/useSchemaUploadHandlers.ts`:
- Around line 105-109: The upload mutation created in useUploadSchemaFile
currently returns a bare mutation and SchemaUploadDialog has no way to trigger
cache updates; modify useSchemaUploadHandlers to either (A) return the mutation
object from useUploadSchemaFile so SchemaUploadDialog can call
mutation.invalidate or onSuccess callbacks, or (B) accept a QueryClient
parameter (or obtain one via useQueryClient) and call
queryClient.invalidateQueries for the relevant keys (e.g., "schema",
"extensions") in the mutation's onSuccess handler inside useUploadSchemaFile;
ensure the symbol useUploadSchemaFile's mutationFn remains, add an onSuccess
that invalidates queries or expose the mutation so SchemaUploadDialog can supply
callbacks.
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/components/SchemaUploadDialog/useSchemaUploadHandlers.ts (1)
31-40: Consider client-side file type validation on drop for better UX.
handleFileDropaccepts any file type. While the server should validate the schema file, drag-and-drop bypasses theacceptattribute on<input>elements in some browsers. Adding a quick client-side check (e.g., verifying.jsonextension orapplication/jsonMIME type) would give immediate feedback rather than waiting for a server round-trip to reject the file.💡 Optional: add file type guard
const handleFileDrop = (files: FileList | null) => { if (!files || files.length === 0) return; if (files?.length === 1) { + const droppedFile = files[0]; + if (droppedFile.type && droppedFile.type !== 'application/json' && !droppedFile.name.endsWith('.json')) { + addNotification('Only JSON schema files are supported', 'InvalidFileType'); + return; + } - setFile({ file: files[0], status: FileStatus.READY }); + setFile({ file: droppedFile, status: FileStatus.READY }); setUploadProgress(0); } else {
3 participants
Description
Adds a Schema Upload dialog to the "OpenGraph Management" admin page that allows users to upload a single OpenGraph schema file at a time. The page and API functionality is currently gated behind the
opengraph_extension_managementfeature flag.Motivation and Context
Resolves BED-6759
How Has This Been Tested?
Additional test files added to cover the new functionality.
Screenshots (optional):
Screen.Recording.2026-02-06.at.10.36.17.AM.mov
Types of changes=
Checklist:
Summary by CodeRabbit
New Features
Tests
Accessibility