feat(storage-browser): integrate browser navigation events #5941

calebpollman · 2024-10-23T15:24:07Z

Description of changes

Issue #, if available

Add support for browser navigation through exposing of event callbacks on the View components and controlled props on StorageBrowser.Provider

add "routed" default-auth/ and managed-auth example apps:
- http://localhost:3000/ui/components/storage/storage-browser/managed-auth/routed
- http://localhost:3000/ui/components/storage/storage-browser/default-auth/routed
integrate useProcessTasks in to UploadView
integrate updated store context/state and remove dead code
add do-not-import-from-here directory containing modules that will be replaced with the new actions API
update task action handlers to receive key on top level input
remove as many duplicated types and temp workarounds as possible
remove useControl in anticipation of useView
add unique id generation to all values kept in state
consolidate store context hooks in to useStore
dedupe store "RESET" action types/improve clarity of other store event types
add remove support to the UploadView tasks

Known issues:

progress rendering on uploads is currently unsupported
breadcrumbs currently render the full prefix, the current data parsing logic will be addressed in a follow up

Description of how you validated changes

Example apps

Checklist

Have read the Pull Request Guidelines
PR description included
yarn test passes and tests are updated/added
PR title and commit messages follow conventional commit syntax
If this change should result in a version bump, changeset added (This can be done after creating the PR.) This does not apply to changes made to docs, e2e, examples, or other private packages.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

changeset-bot · 2024-10-23T15:24:11Z

⚠️ No Changeset found

Latest commit: a5b5816

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

...ages/react-storage/src/components/StorageBrowser/views/LocationActionView/UploadControls.tsx

cshfang · 2024-10-23T23:32:39Z

packages/react-storage/src/components/StorageBrowser/StorageBrowserDefault.tsx

@@ -12,16 +12,14 @@ import { useControl } from './context/control';
 export function StorageBrowserDefault(): React.JSX.Element {
  const { LocationActionView, LocationDetailView, LocationsView } = useViews();

-  const [{ location }] = useControl('NAVIGATE');
-  const [{ selected }] = useControl('LOCATION_ACTIONS');
+  const [{ actionType, history }] = useStore();


packages/react-storage/src/components/StorageBrowser/StorageBrowserDefault.tsx

cshfang · 2024-10-23T23:35:53Z

packages/react-storage/src/components/StorageBrowser/actions/configs/context.ts

@@ -7,5 +7,9 @@ export interface ActionConfigsProviderProps {

 const defaultValue: ActionConfigs = {};

+export interface ActionConfigsProviderProps {
+  actions?: ActionConfigs;


If the type is ActionConfigs, shouldn't the prop just match? i.e. actionConfigs?:

Usually yes but this will most like be actions on the createStorageBrowser API but also okay with changing it here

Oof. What is the DX going to look like there? I don't know if I love providing configuration via some API property that does not imply configuration. Especially when it then unexpectedly translates to configuration internally - one more mental mapping we have to keep track of, albeit just early on in the sequence

It's in a config object, can discuss offline

cshfang · 2024-10-23T23:53:11Z

packages/react-storage/src/components/StorageBrowser/actions/handlers/copy.ts

@@ -19,9 +19,10 @@ export interface CopyHandlerOutput extends TaskHandlerOutput {}
 export interface CopyHandler
  extends TaskHandler<CopyHandlerInput, CopyHandlerOutput> {}

-export const copyHandler: CopyHandler = ({ config, options, prefix, data }) => {
+export const copyHandler: CopyHandler = (input) => {
+  const { config, key, options, prefix, data } = input;


Nit: What was the issue with just destructuring the param?

Personal preference to keep them on one line 🤷

packages/react-storage/src/components/StorageBrowser/actions/handlers/utils.ts

cshfang · 2024-10-24T00:12:23Z

packages/react-storage/src/components/StorageBrowser/actions/handlers/utils.ts

+    }
+  }
+
+  return { bucket, id, permission, prefix, type };


Nit: Could return within the cases to avoid having to break or use let

That's what i normally what i would do here but am trying to actually use let instead of forcing const everywhere 😓

Hmm I'm curious what the rationale for that is. let is a perfectly acceptable pattern when it is necessary but why wouldn't we discourage it when it isn't necessary but does introduce potential sharp edges?

Not quite sure i see sharp edges here, would be more concerned if the assignment was an array or object

cshfang · 2024-10-24T00:14:08Z

packages/react-storage/src/components/StorageBrowser/actions/handlers/utils.ts

+    }
+    case 'PREFIX': {
+      // { scope: 's3://bucket/path/*', type: 'PREFIX', },
+      bucket = sanitizedScope.slice(0, sanitizedScope.indexOf('/'));


Is it possible for indexOf to return -1? (and is that desirable?)

Good question - we have an open issue to filter out PREFIX locations that do not end with a trailing slash which should prevent the possibility of indexOf resolving to -1. Probably makse sense to address it here tho as well, can address in a follow up

cshfang · 2024-10-24T00:14:49Z

packages/react-storage/src/components/StorageBrowser/actions/types.ts

@@ -1,6 +1,6 @@
 import { LocationCredentialsProvider } from '../storage-internal';

-import { ActionState } from '../context/actions/createActionStateContext';
+import { ActionState } from '../do-not-import-from-here/actions/createActionStateContext';


calebpollman · 2024-10-24T00:24:16Z

examples/next/pages/ui/components/storage/storage-browser/composable-playground/index.page.tsx

-  const [{ selected }] = useControl('LOCATION_ACTIONS');
+  const [type, setActionType] = React.useState<string | undefined>(undefined);


useControl has been removed from createStorageBrowser and will be replaced with useView. In the meantime updated this app to work with the new event callbacks

calebpollman · 2024-10-24T00:25:51Z

examples/next/pages/ui/components/storage/storage-browser/default-auth/routed/useIsSignedIn.ts

+ * @param {UseIsSignedInParams} params `onSignIn` and `onSignOut` event callbacks
+ * @returns {UseIsSignedIn} Outputs `isSignedIn`
+ */
+export default function useIsSignedIn({


Copied from an internal file

calebpollman · 2024-10-24T00:39:51Z

packages/react-storage/src/components/StorageBrowser/actions/handlers/utils.ts

@@ -50,3 +52,43 @@ export const constructBucket = ({
  bucketName: string;
  region: string;
 } => ({ bucketName, region });
+
+export const parseLocationAccess = (location: LocationAccess): LocationData => {


Migrated with existing unit tests from a deleted module

calebpollman · 2024-10-24T00:44:36Z

packages/react-storage/src/components/StorageBrowser/adapters/types.ts

 import {
  AuthConfigAdapter,
  CreateManagedAuthConfigAdapterInput,
 } from '../storage-internal';

 export interface StorageBrowserAuthAdapter extends AuthConfigAdapter {
+  accountId?: string;


Added accountId to the return of the adapters to allow for it to passthrough directly to the input of createStorageBrowser

calebpollman · 2024-10-24T00:50:44Z

packages/react-storage/src/components/StorageBrowser/providers/store/StoreProvider.tsx

-        <LocationItemsProvider locationItems={locationItems}>
+        <LocationItemsProvider>


Removed locationItems from the StorageBrowser.Provider interface, can be revisited if it makes sense to add it

calebpollman · 2024-10-24T00:55:23Z

packages/react-storage/src/components/StorageBrowser/views/Controls/Navigate.tsx

+            // @todo: prefix should not be required to refresh
+            prefix: current?.prefix ?? '',
+            options: { reset: true },


This needs to be addressed in a follow up when moving to the actions API

calebpollman · 2024-10-24T00:56:59Z

...eact-storage/src/components/StorageBrowser/views/LocationActionView/CreateFolderControls.tsx

    // reset hook state on exit, use empty string for prefix to keep TS happy
+    // @todo: this needs to be addressed


Previously noted, but needs to be addressed with the actions API migration

calebpollman · 2024-10-24T00:59:49Z

...ages/react-storage/src/components/StorageBrowser/views/LocationActionView/UploadControls.tsx

+            <button
+              onClick={() => {
+                row.remove();
+              }}
+            >
+              Remove
+            </button>


Just added an HTML button as this will need to be replaced during DataTable integration

...ages/react-storage/src/components/StorageBrowser/views/LocationActionView/UploadControls.tsx

calebpollman · 2024-10-24T01:02:15Z

...ages/react-storage/src/components/StorageBrowser/views/LocationActionView/UploadControls.tsx

+        _remove();
+      };
+
+      return { ...task, folder, key, progress: 0, remove, size, type };


progress support is currently missing and will be addressed in a targeted follow to useProcessTasks

...omponents/StorageBrowser/providers/configuration/__tests__/useGetActionInputCallback.spec.ts

cshfang · 2024-10-24T16:37:42Z

packages/react-storage/src/components/StorageBrowser/providers/store/useStore.ts

+
+  const dispatchHandler: HandleStoreAction = React.useCallback(
+    (action) => {
+      switch (action.type) {


Nit: Is type guaranteed to be unique by the typing? Marking this nit since it's kind of an edge case either way.

packages/react-storage/src/components/StorageBrowser/tasks/useProcessTasks.ts

packages/react-storage/src/components/StorageBrowser/views/Controls/Download.tsx

packages/react-storage/src/components/StorageBrowser/views/Controls/Navigate.tsx

calebpollman · 2024-10-24T20:43:47Z

packages/react-storage/jest.config.ts

-      functions: 83,
-      lines: 91,
-      statements: 90,
+      branches: 80,


Minor drop in branch coverage but will be addressed in other migration PRs

calebpollman · 2024-10-24T20:44:55Z

packages/react-storage/src/components/StorageBrowser/tasks/__tests__/utils.spec.ts

@@ -74,15 +74,3 @@ describe('isCancelableOutput', () => {
    expect(result).toBe(false);
  });
 });
-
-describe('hasExistingTask', () => {


No longer exists

calebpollman · 2024-10-24T20:48:59Z

...age/src/components/StorageBrowser/views/LocationActionView/__tests__/UploadControls.spec.tsx

-  it('opens the file picker when the add files button is clicked and uses the file selection dialog', async () => {
-    const user = userEvent.setup();
-    const files = [
-      new File(['content1'], 'file1.txt', { type: 'text/plain' }),
-      new File(['content2'], 'file2.txt', { type: 'text/plain' }),
-      new File(['content3'], 'file3.txt', {
-        type: 'text/plain',
-      }),
-    ];
-
-    render(
-      <Provider>
-        <UploadControls />
-      </Provider>
-    );
-
-    const button = screen.getByRole('button', { name: 'Add files' });
-    const input: HTMLInputElement = screen.getByTestId('amplify-file-select');
-
-    expect(input).toHaveAttribute('multiple');
-
-    await act(async () => {
-      await user.click(button);
-      await user.upload(input, files);
-    });
-
-    expect(input.files).toHaveLength(3);
-  });
-
-  it('adds files dragged into the drop zone to the file list', async () => {
-    const files = [new File(['content'], 'file.txt', { type: 'text/plain' })];
-
-    render(
-      <Provider>
-        <UploadControls />
-      </Provider>
-    );
-
-    const dropzone = screen.getByTestId('storage-browser-table');
-
-    fireEvent.drop(dropzone, {
-      dataTransfer: {
-        files,
-      },
-    });
-
-    await waitFor(() => {
-      expect(screen.getByText('file.txt')).toBeInTheDocument();
-    });
-  });
-
-  it('has the webkitdirectory attribute for the input select for folders', () => {
-    render(
-      <Provider>
-        <UploadControls />
-      </Provider>
-    );
-
-    const input: HTMLInputElement = screen.getByTestId('amplify-folder-select');
-
-    expect(input).toHaveAttribute('webkitdirectory');
-  });


Removed these tests as they were testing the logic of underlying components and will be covered by the upcoming DropZone composable

calebpollman · 2024-10-24T20:49:57Z

...t-storage/src/components/StorageBrowser/views/LocationDetailView/__tests__/Controls.spec.tsx

Moved these in to their own test file from /LocationDetailView.spec.tsx

calebpollman force-pushed the feat-storage-browser/browser-navigation branch 6 times, most recently from 3a852c4 to 986d37d Compare October 23, 2024 23:49

github-advanced-security bot found potential problems Oct 23, 2024

View reviewed changes

...ages/react-storage/src/components/StorageBrowser/views/LocationActionView/UploadControls.tsx Fixed Show resolved Hide resolved

calebpollman force-pushed the feat-storage-browser/browser-navigation branch from 986d37d to 8ba14e6 Compare October 24, 2024 00:20

cshfang reviewed Oct 24, 2024

View reviewed changes

calebpollman added 2 commits October 23, 2024 18:06

feat(storage-browser): integrate browser navigation

7df9f79

feat(storage-browser): add routed example app

65bb318

calebpollman force-pushed the feat-storage-browser/browser-navigation branch from 8ba14e6 to 65bb318 Compare October 24, 2024 01:06

calebpollman commented Oct 24, 2024

View reviewed changes

calebpollman changed the title ~~[WIP] feat(storage-browser): integrate browser navigation events~~ feat(storage-browser): integrate browser navigation events Oct 24, 2024

calebpollman marked this pull request as ready for review October 24, 2024 01:11

calebpollman requested a review from a team as a code owner October 24, 2024 01:11

cshfang reviewed Oct 24, 2024

View reviewed changes

cshfang previously approved these changes Oct 24, 2024

View reviewed changes

packages/react-storage/src/components/StorageBrowser/views/Controls/Download.tsx Show resolved Hide resolved

packages/react-storage/src/components/StorageBrowser/views/Controls/Navigate.tsx Show resolved Hide resolved

fix unit tests

ff1ebdb

calebpollman dismissed cshfang’s stale review via ff1ebdb October 24, 2024 20:42

calebpollman commented Oct 24, 2024

View reviewed changes

calebpollman added 2 commits October 24, 2024 14:10

remove unused test utilities

b4e4c4b

remove testUtils pattern from jest coverage exclusion

99be6e8

calebpollman temporarily deployed to ci October 24, 2024 21:23 — with GitHub Actions Inactive

calebpollman had a problem deploying to ci October 24, 2024 21:23 — with GitHub Actions Failure

calebpollman temporarily deployed to ci October 24, 2024 21:23 — with GitHub Actions Inactive

fix navigation e2e test

a5b5816

calebpollman temporarily deployed to ci October 24, 2024 23:26 — with GitHub Actions Inactive

cshfang approved these changes Oct 24, 2024

View reviewed changes

reesscot approved these changes Oct 25, 2024

View reviewed changes

calebpollman merged commit 9881be4 into feat-storage-browser/main Oct 25, 2024
32 checks passed

calebpollman deleted the feat-storage-browser/browser-navigation branch October 25, 2024 00:05

calebpollman mentioned this pull request Oct 25, 2024

chore(storage-browser): minor follow ups to browser nav support integration #5965

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage-browser): integrate browser navigation events #5941

feat(storage-browser): integrate browser navigation events #5941

calebpollman commented Oct 23, 2024 •

edited

Loading

changeset-bot bot commented Oct 23, 2024 •

edited

Loading

cshfang Oct 23, 2024

cshfang Oct 23, 2024

calebpollman Oct 24, 2024

cshfang Oct 24, 2024

calebpollman Oct 24, 2024

cshfang Oct 23, 2024

calebpollman Oct 24, 2024

cshfang Oct 24, 2024

calebpollman Oct 24, 2024

cshfang Oct 24, 2024

calebpollman Oct 24, 2024

cshfang Oct 24, 2024

calebpollman Oct 24, 2024

cshfang Oct 24, 2024

calebpollman Oct 24, 2024

calebpollman Oct 24, 2024

calebpollman Oct 24, 2024

calebpollman Oct 24, 2024

calebpollman Oct 24, 2024

calebpollman Oct 24, 2024

calebpollman Oct 24, 2024

calebpollman Oct 24, 2024

calebpollman Oct 24, 2024

cshfang Oct 24, 2024

calebpollman Oct 24, 2024

calebpollman Oct 24, 2024

calebpollman Oct 24, 2024

calebpollman Oct 24, 2024

		const [{ selected }] = useControl('LOCATION_ACTIONS');
		const [type, setActionType] = React.useState<string \| undefined>(undefined);

		<LocationItemsProvider locationItems={locationItems}>
		<LocationItemsProvider>

		// reset hook state on exit, use empty string for prefix to keep TS happy
		// @todo: this needs to be addressed

feat(storage-browser): integrate browser navigation events #5941

feat(storage-browser): integrate browser navigation events #5941

Conversation

calebpollman commented Oct 23, 2024 • edited Loading

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

changeset-bot bot commented Oct 23, 2024 • edited Loading

⚠️ No Changeset found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebpollman commented Oct 23, 2024 •

edited

Loading

changeset-bot bot commented Oct 23, 2024 •

edited

Loading