Skip to content

New Category switcher#14936

Open
farmaazon wants to merge 11 commits intodevelopfrom
wip/farmaazon/categories
Open

New Category switcher#14936
farmaazon wants to merge 11 commits intodevelopfrom
wip/farmaazon/categories

Conversation

@farmaazon
Copy link
Copy Markdown
Contributor

Pull Request Description

Fixes #14880

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • [ ] If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

@farmaazon farmaazon self-assigned this Apr 2, 2026
@farmaazon farmaazon changed the title Wip/farmaazon/categories New Category switcher Apr 9, 2026
@farmaazon farmaazon force-pushed the wip/farmaazon/categories branch from 5da7d20 to e78b0aa Compare April 9, 2026 09:41
@farmaazon farmaazon force-pushed the wip/farmaazon/categories branch from e78b0aa to ef27359 Compare April 9, 2026 10:51

function onDragover(event: DragEvent) {
for (const item of event.dataTransfer?.items ?? []) {
if (acceptedDragTypes.value.find((type) => type === item.type)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array.includes?

Comment on lines +91 to +93
payloads[0]?.items.length === 1 && firstItem != null ?
getText('deleteSelectedAssetActionText', firstItem.title)
: getText('deleteSelectedAssetsActionText', payloads.flatMap(({ items }) => items).length),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

firstItem != null seems redundant, as there's a guard above.

Why is the length computation in the the condition different from how the number is computed in the plural case? This looks like it could use the plural message when the count is 1.


function addLocalDirectory(path: Path) {
const state = localDirectoryStore.getState()
if (!state.localDirectories.find((p) => p === path)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

includes?

Comment on lines +255 to +258
const index = state.localDirectories.findIndex((p) => p === path)
if (index >= 0) {
const newList = [...state.localDirectories]
newList.splice(index, 1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array.filter?

Comment on lines +308 to +309
if (isCloudCategory(from) || isCloudCategory(to)) {
if (isLocalCategory(from) || isLocalCategory(to)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isCloudCategory(from) !== isCloudCategory(to)?

return 'move'
case 'local':
case 'localDirectory':
return isCloudCategory(to) ? 'copy' : 'move'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the copy case here not already covered by the cloud/local check above?

refetchInterval: null,
}),
)
return [...nonDeletedAssets.assets, ...deletedAssets.assets] as const
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These could be pipelined with Promise.all

type AssetId,
type DirectoryId,
} from 'enso-common/src/services/Backend'
// import { parseDirectoriesPath } from 'enso-common/src/services/Backend/utilities'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented

Comment on lines 37 to 47
const targetDirectoryIndex = finalPath.findIndex(({ id }) => id === targetDirectory)
const targetDirectoryInfo = finalPath[targetDirectoryIndex]
if (targetDirectoryIndex === -1 || !targetDirectoryInfo) {
return
}
const pathToDirectory = finalPath
.slice(0, targetDirectoryIndex + 1)
.map(({ id, categoryId }) => ({ id, categoryId }))
.map(({ id, category }) => ({ id, category }))
const rootDirectoryInThePath = pathToDirectory[0]
// This should never happen, as we always have the root directory in the path.
invariant(rootDirectoryInThePath, 'Root directory id is null')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of this logic seems unused since we only use one field of the first element of pathToDirectory

@farmaazon farmaazon added CI: No changelog needed Do not require a changelog entry for this PR. and removed CI: No changelog needed Do not require a changelog entry for this PR. labels Apr 10, 2026
@farmaazon farmaazon requested a review from kazcw April 10, 2026 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: No changelog needed Do not require a changelog entry for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Collapsed categories in Drive view.

2 participants