Skip to content

fix: reopens files that were opened when dragging and dropping files or folders#1704

Open
tarikutk wants to merge 6 commits into
sassoftware:mainfrom
tarikutk:fix/drag/drop
Open

fix: reopens files that were opened when dragging and dropping files or folders#1704
tarikutk wants to merge 6 commits into
sassoftware:mainfrom
tarikutk:fix/drag/drop

Conversation

@tarikutk

@tarikutk tarikutk commented Nov 19, 2025

Copy link
Copy Markdown
Contributor

Summary:

  1. it opens previously opened files when dragging and dropping a file or a folder.
  2. it prevents VSCode from opening a folder as a file when moving a folder from one folder to another folder. it instead opens the file that was opened in that folder

Testing:
verified it opens previously opened files instead of opening a folder as a file.

Fixes #1478

@tarikutk tarikutk changed the title fix: opens closed file not the folder in drag/drop fix: reopens files that were opened when dragging and dropping files or folders Nov 19, 2025
@tarikutk tarikutk marked this pull request as ready for review November 20, 2025 04:03

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes file reopening behavior when dragging and dropping files or folders in the Content Navigator. It ensures that previously opened files are reopened at their new locations after a move operation, and prevents VSCode from incorrectly opening folders as files.

Key Changes:

  • Modified the moveTo handling logic to track closed files as an array instead of a boolean
  • Added a new calculateNewFileUri method to compute new URIs for files that were moved
  • Implemented logic to reopen all previously opened files after a successful move operation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread client/src/components/ContentNavigator/ContentDataProvider.ts Outdated
Comment thread client/src/components/ContentNavigator/ContentDataProvider.ts Outdated
Comment thread client/src/components/ContentNavigator/ContentDataProvider.ts Outdated
Comment thread client/src/components/ContentNavigator/ContentDataProvider.ts
Comment thread client/src/components/ContentNavigator/ContentDataProvider.ts Outdated
Comment thread client/src/components/ContentNavigator/ContentDataProvider.ts Outdated
Comment thread client/src/components/ContentNavigator/ContentDataProvider.ts Outdated
Comment thread client/src/components/ContentNavigator/ContentDataProvider.ts Outdated
Comment thread client/src/components/ContentNavigator/ContentDataProvider.ts Outdated
Comment thread client/src/components/ContentNavigator/ContentDataProvider.ts Outdated
@scottdover

Copy link
Copy Markdown
Contributor

Hey @tarikutk . Is there a github issue associated with this PR? Or rather, what issue is this solving?

Comment thread client/src/components/ContentNavigator/ContentDataProvider.ts Outdated
isContainer as getIsContainer,
} from "./utils";

const SAS_FILE_SEPARATOR = "~fs~";

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.

Fwiw, this is sas server specific. Anything that is specific to connection type should happen in the adapter if possible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

okay. I updated that. I am importing from the adapter instead.

@tarikutk

tarikutk commented Dec 8, 2025

Copy link
Copy Markdown
Contributor Author

Hey @tarikutk . Is there a github issue associated with this PR? Or rather, what issue is this solving?

yes @scottdover, there is a GitHub issue associated with it. I attached the link now in the PR message. it is #1478.

@tarikutk tarikutk requested a review from scottdover December 8, 2025 01:41
@scottdover

scottdover commented Jan 15, 2026

Copy link
Copy Markdown
Contributor

Sorry for the delay in getting back to this one. At a high level, this is how the code is structured w/r/t file access.

  • ContentDataProvider exposes the data view to the ui via a treeview
  • ContentModel is responsible for handling the business logic related to file CRUD operations
  • ContentAdapter is an interface that defines the specific methods used in ContentModel to provide data back to the user
  • There are a few implementations of ContentAdapter
    • ItcServerAdapter: For server files via an itc connection
    • RestServerAdapter: For server files via a rest (viya) connection
    • RestContentAdapter: For sas content via a rest (viya) connection

Your implementation has itc (relativePath.replace(/^~fs~/, "") as an example) & rest specific code (/(\/files\/[^&]*)/ as an example) in ContentDataProvider. Any code that is specific to a certain connection type should be nested in the relevant adapter.

@tarikutk tarikutk force-pushed the fix/drag/drop branch 2 times, most recently from 8d2e0f0 to 49b6eeb Compare February 18, 2026 12:24
@tarikutk

Copy link
Copy Markdown
Contributor Author

I updated @scottdover .

@tarikutk

Copy link
Copy Markdown
Contributor Author

the commit for itcServerAdapter is something which I was not able to test. and the last one is just to fix typescript issues. please let me know if itcServerAdapter works and if the fix for typescript breaks things. I will revert those if needed. Thanks

@scottdover scottdover requested review from smorrisj and removed request for kpatl1 February 20, 2026 18:28
Comment thread client/src/components/ContentNavigator/ContentDataProvider.ts Outdated
Comment thread client/src/components/ContentNavigator/ContentDataProvider.ts Outdated
item: ContentItem,
targetParentFolderUri: string,
) => Promise<Uri | undefined>;
calculateNewFileUri?: (

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.

Should this be optional? It should if this isn't an issue that affects SAS content. If it does impact sas content, I would expect to see changes in RestContentAdapter as well

Also, super nitpicky, but these functions for this type are all alphabetized. Will you alphabetize this new function?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes. I think it should be optional. restContentAdapter is not calling it at all because closeFileIfOpen is returning true rather than array of closed files for restContentAdapter.

}
}

public calculateNewFileUri(

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.

It feels like there's a lot of duplication between this and the rest server adapter implementation. From what I could tell, the three pieces that are unique for this function and the other are:

  • How directory separators are determined
  • how to extractPathFromUri
  • how to generateNewFilePath (basically, lines 352-371 here, 482-505 in the other file)

With that in mind, I'm wondering if this method can look more like this...

const extractPathFromUri = (uri: string) => {
      /* process things */
      return '';
    };
    const getDirectorySeparator = (basePath: string) => {
      /*
      use getDirectorySeparator in ItcServerAdapter, or return
      the separator for rest
      */
    };
    const generateNewFilePath = (
      oldBasePath: string,
      closedFilePath: string,
      newItemUri: Uri
    ) => {
      /* Do specific calculation for ITC or REST */
    };

    return calculateNewFileUri(
      extractPathFromUri,
      getDirectorySeparator,
      generateNewFilePath,
    );

With the above, extractPathFromUri, getDirectorySeparator, and generateNewFilePath are unique for ItcServerAdapter or RestServerAdapter are unique, while the code moved to a shared calculateNewFileUri function would contain the duplicated bits (NOTE: This is an oversimplification to some extent and there may need to be more parameters passed into the shared function)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I broke down the functions to utils instead of passing those functions as a prop. passing 6 parameters and passing 3 of them back to those functions was not looking good. I added them in utils of a specific adapter instead.

tarikutk added 6 commits March 9, 2026 15:12
…or folders

Signed-off-by: tariku <tarikutadese24@gmail.com>
Signed-off-by: tariku <tarikutadese24@gmail.com>
Signed-off-by: tariku <tarikutadese24@gmail.com>
Signed-off-by: tariku <tarikutadese24@gmail.com>
Signed-off-by: tariku <tarikutadese24@gmail.com>
Signed-off-by: tariku <tarikutadese24@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

File folder will be opened as a file when drag/drop it to a new position on SAS SERVER

3 participants