Uh oh!
There was an error while loading. Please reload this page.
blob_showcase_infra - #314
Open
dani-vaibhav wants to merge 2 commits into
Open
Conversation
Contributor
Reviewer's GuideAdds a tech-admin Blob Storage Explorer feature spanning backend, GraphQL, and UI layers, including new hierarchical blob listing, container enumeration, bounded blob download, and permission-gated access for staff users. Sequence diagram for techAdminBlobList hierarchical listingsequenceDiagram
actor StaffUser
participant BlobStorageExplorerContainer
participant GraphQLServer
participant TechAdminResolvers
participant TechAdminApplicationService as TechAdminService
participant BlobStorageOperations as BlobStorageService
StaffUser->>BlobStorageExplorerContainer: Select container / Apply filters
BlobStorageExplorerContainer->>GraphQLServer: TechAdminBlobStorageExplorerList
GraphQLServer->>TechAdminResolvers: techAdminBlobList(args)
TechAdminResolvers->>TechAdminResolvers: assertCanViewBlobExplorer(context)
TechAdminResolvers->>TechAdminResolvers: buildBlobListQueryCommand(args)
TechAdminResolvers->>TechAdminApplicationService: TechAdminService.ListBlobHierarchy(command)
TechAdminApplicationService->>BlobStorageOperations: listBlobHierarchy(command)
BlobStorageOperations-->>TechAdminApplicationService: BlobHierarchyPage
TechAdminApplicationService-->>TechAdminResolvers: BlobHierarchyPage
TechAdminResolvers-->>GraphQLServer: BlobHierarchyPage mapped to GraphQL types
GraphQLServer-->>BlobStorageExplorerContainer: techAdminBlobList result
BlobStorageExplorerContainer-->>StaffUser: Render folders, blobs, continuationToken
Sequence diagram for techAdminBlobContent preview and SAS download URLsequenceDiagram
actor StaffUser
participant BlobStorageExplorer as BlobStorageExplorerUI
participant BlobStorageExplorerContainer
participant GraphQLServer
participant TechAdminResolvers
participant TechAdminApplicationService as TechAdminService
participant BlobStorageOperations as BlobStorageService
participant ClientUploadOperations as ClientUploadService
StaffUser->>BlobStorageExplorer: Click View on blob
BlobStorageExplorer->>BlobStorageExplorerContainer: onViewBlob(blob)
BlobStorageExplorerContainer->>GraphQLServer: TechAdminBlobStorageExplorerContent
GraphQLServer->>TechAdminResolvers: techAdminBlobContent(args)
TechAdminResolvers->>TechAdminResolvers: assertCanViewBlobExplorer(context)
TechAdminResolvers->>TechAdminApplicationService: TechAdminService.GetBlobContent({containerName, blobName})
TechAdminApplicationService->>BlobStorageOperations: downloadBlob({containerName, blobName})
BlobStorageOperations-->>TechAdminApplicationService: BlobDownloadResult
TechAdminApplicationService->>ClientUploadOperations: generateReadSasToken({containerName, blobName, expiresOn})
ClientUploadOperations-->>TechAdminApplicationService: sasToken
TechAdminApplicationService-->>TechAdminResolvers: BlobContentResult (contentBase64, metadata, tags, downloadUrl)
TechAdminResolvers-->>GraphQLServer: BlobContent
GraphQLServer-->>BlobStorageExplorerContainer: techAdminBlobContent result
BlobStorageExplorerContainer-->>BlobStorageExplorer: preview
BlobStorageExplorer-->>StaffUser: Show preview and Download action
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
listBlobHierarchyPage, the folder loop skips all folders whenrequest.metadataKeyis set (if (request.metadataKey?.trim()) continue;), which contradicts the comment that folders should still appear for navigation; consider either removing this condition or aligning the comment and behavior so filtered listings still expose folder paths as intended. - The
BlobStorageExplorercomponent creates object URLs for previews viaURL.createObjectURLbut never revokes them, which can leak memory over time; wrap this in auseEffectthat revokes the URL in a cleanup function when the preview changes or the component unmounts. BlobStorageExplorermaintains its owndraftFiltersstate while the container also tracks filters and passesfilters/onChangeFilters, leading to duplicated and potentially out-of-sync state; simplifying this so the container is the single source of truth (and the presentational component is fully controlled) will make filter behavior easier to reason about.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments- In `listBlobHierarchyPage`, the folder loop skips all folders when `request.metadataKey` is set (`if (request.metadataKey?.trim()) continue;`), which contradicts the comment that folders should still appear for navigation; consider either removing this condition or aligning the comment and behavior so filtered listings still expose folder paths as intended.
- The `BlobStorageExplorer` component creates object URLs for previews via `URL.createObjectURL` but never revokes them, which can leak memory over time; wrap this in a `useEffect` that revokes the URL in a cleanup function when the preview changes or the component unmounts.
-`BlobStorageExplorer` maintains its own `draftFilters` state while the container also tracks filters and passes `filters`/`onChangeFilters`, leading to duplicated and potentially out-of-sync state; simplifying this so the container is the single source of truth (and the presentational component is fully controlled) will make filter behavior easier to reason about.
## Individual Comments### Comment 1
<locationpath="packages/ocom/ui-staff-route-tech-admin/src/components/blob-storage-explorer.tsx"line_range="165" />
<code_context>
+ onViewBlob,
+ onClosePreview,
+}) => {
+ const [draftFilters, setDraftFilters] = useState(filters);
++ const breadcrumbItems = useMemo(() => {
</code_context>
<issue_to_address>
**issue (bug_risk):** Local filter state is not synced with `filters` prop, leading to potential stale UI when parent resets filters.
`draftFilters` is only initialized from `filters` and never updated when `filters` changes, so the component and its parent can diverge (e.g. after a parent reset or state restore). Consider either making `BlobStorageExplorer` fully controlled (use `filters` directly and call `onChangeFilters` on every change), or syncing `draftFilters` with `filters` via `useEffect(() => setDraftFilters(filters), [filters])` so external updates are reflected.
</issue_to_address>
### Comment 2
<locationpath="packages/ocom/ui-staff-route-tech-admin/src/components/blob-storage-explorer.tsx"line_range="291-300" />
<code_context>
+},+];
++ const previewObjectUrl = useMemo(() => {
+if (!preview?.contentBase64) {+ return null;+}+if (!isImageContentType(preview.contentType) && !isPdfContentType(preview.contentType)) {+ return null;+}+const binary = atob(preview.contentBase64);+const bytes = Uint8Array.from(binary, (char) => char.charCodeAt(0));+const blob = new Blob([bytes], { type: preview.contentType ?? 'application/octet-stream' });+return URL.createObjectURL(blob);+ }, [preview]);
++ const handleDownload = () => {
</code_context>
<issue_to_address>
**issue (performance):** Object URL created for previews is never revoked, which can leak memory over time.
`previewObjectUrl` is created with `URL.createObjectURL` but never revoked, so repeated preview opens in a long-lived session can accumulate blob URLs and increase memory usage. Consider cleaning it up in an effect that runs when the URL changes/unmounts, e.g.:
```tsconst previewObjectUrl =useMemo(() => {
if (!preview?.contentBase64) returnnull;
if (!isImageContentType(preview.contentType) &&!isPdfContentType(preview.contentType)) returnnull;
const binary =atob(preview.contentBase64);
const bytes =Uint8Array.from(binary, (char) =>char.charCodeAt(0));
const blob =newBlob([bytes], { type: preview.contentType??'application/octet-stream' });
returnURL.createObjectURL(blob);
}, [preview]);
useEffect(() => () => {
if (previewObjectUrl) URL.revokeObjectURL(previewObjectUrl);
}, [previewObjectUrl]);
```
</issue_to_address>
### Comment 3
<locationpath="packages/cellix/service-blob-storage/tests/index.test.ts"line_range="272-281" />
<code_context>
+ expect(result).toEqual([{ name: 'member-assets' }, { name: 'private' }]);+});++it('lists one hierarchy level with folders, blob properties, and a continuation token', async () => {+ const service = new ServiceBlobStorage({ accountName });+ await service.startUp();++ const result = await service.listBlobHierarchy({+ containerName: 'member-assets',+ prefix: '',+ pageSize: 20,+ });++ expect(listBlobsByHierarchyMock).toHaveBeenCalledWith('/', {+ prefix: undefined,+ includeMetadata: true,
</code_context>
<issue_to_address>
**suggestion (testing):** Hierarchy listing assertion is over‑specific about the `prefix` option and may become brittle.
In the `lists one hierarchy level with folders...` test, this assertion couples the test to the exact options shape. If the implementation simply omits a falsy `prefix`, the behaviour remains correct but this test will fail. To make it more robust, assert only the relevant fields:
```tsexpect(listBlobsByHierarchyMock).toHaveBeenCalledWith(
'/',
expect.objectContaining({
includeMetadata: true,
includeTags: true,
}),
);
```
You can then add a separate test with a non‑empty prefix that explicitly checks the `prefix` value is forwarded.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| onViewBlob, | ||
| onClosePreview, | ||
| }) => { | ||
| const [draftFilters, setDraftFilters] = useState(filters); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Local filter state is not synced with filters prop, leading to potential stale UI when parent resets filters.
draftFilters is only initialized from filters and never updated when filters changes, so the component and its parent can diverge (e.g. after a parent reset or state restore). Consider either making BlobStorageExplorer fully controlled (use filters directly and call onChangeFilters on every change), or syncing draftFilters with filters via useEffect(() => setDraftFilters(filters), [filters]) so external updates are reflected.
Comment on lines
+291
to
+300
| const previewObjectUrl = useMemo(() => { | ||
| if (!preview?.contentBase64) { | ||
| return null; | ||
| } | ||
| if (!isImageContentType(preview.contentType) && !isPdfContentType(preview.contentType)) { | ||
| return null; | ||
| } | ||
| const binary = atob(preview.contentBase64); | ||
| const bytes = Uint8Array.from(binary, (char) => char.charCodeAt(0)); | ||
| const blob = new Blob([bytes], { type: preview.contentType ?? 'application/octet-stream' }); |
Contributor
There was a problem hiding this comment.
issue (performance): Object URL created for previews is never revoked, which can leak memory over time.
previewObjectUrl is created with URL.createObjectURL but never revoked, so repeated preview opens in a long-lived session can accumulate blob URLs and increase memory usage. Consider cleaning it up in an effect that runs when the URL changes/unmounts, e.g.:
constpreviewObjectUrl=useMemo(()=>{if(!preview?.contentBase64)returnnull;if(!isImageContentType(preview.contentType)&&!isPdfContentType(preview.contentType))returnnull;constbinary=atob(preview.contentBase64);constbytes=Uint8Array.from(binary,(char)=>char.charCodeAt(0));constblob=newBlob([bytes],{type: preview.contentType??'application/octet-stream'});returnURL.createObjectURL(blob);},[preview]);useEffect(()=>()=>{if(previewObjectUrl)URL.revokeObjectURL(previewObjectUrl);},[previewObjectUrl]);Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Enable authorized staff to explore, preview, and download Azure Blob Storage content through the Tech Admin portal while hardening local test-server startup and port management.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests: