Skip to content

Conversation

@kpatl1
Copy link
Collaborator

@kpatl1 kpatl1 commented Aug 14, 2025

Summary

This change improves the user experience in the SAS Content Navigator by showing full hierarchical paths when hovering over files and folders, instead of just displaying the filename.

Key Changes:

  1. Modified ContentDataProvider.getTreeItem() to fetch and display full path information in tooltips
  2. Added fallback behavior - if full path retrieval fails, tooltips gracefully degrade to showing just the item name

Before:

  • Hovering over a file showed: /test1.sas

After:

  • Hovering over a file shows: /My Folder/Subfolder/test1.sas

Testing

-Verified tooltips show full paths for files in nested folder structures

Fixes #1577

@kpatl1 kpatl1 requested a review from scnwwu August 14, 2025 18:30
@kpatl1 kpatl1 marked this pull request as ready for review August 14, 2025 18:30
Copy link
Contributor

@scottdover scottdover left a comment

Choose a reason for hiding this comment

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

Hey @kpatl1! Thanks for taking a look at this. Only a couple things I noticed...

  • If you hover over SAS Server, it'll show a tooltip that isn't quite right. We could honestly probably update the implementation in RestServerAdapter.getPathOfItem and ItcServerAdapter.getPathOfItem to return an empty string for the root folder
  • Currently, the recycle bin has a path tooltip. Is this expected (is that a valid path)?
  • Last, the only concern I have is that this will be called once per file or folder. So if we have a folder w/ 50 files, that's 51 requests to load the files (looks like this is only an issue w/ sas content). Have we tested how this impacts things if we have lots of files (i.e. btw 50-100 files)? The good thing is vscode does a decent job at caching these items so we shouldn't call it over and over. This is probably a non-issue, but just wanted to call it out for resting

@scottdover
Copy link
Contributor

Thinking about it further, you can just check if the path can be copied before you show the tooltip (i.e. checking item.contextValue.includes('copyPath'))

Kishan Patel added 2 commits August 14, 2025 17:38
I, Kishan Patel <kishan.b.patel@sas.com>, hereby add my Signed-off-by to this commit: c0622a9
I, Kishan Patel <kishan.b.patel@sas.com>, hereby add my Signed-off-by to this commit: 64965fe

Signed-off-by: Kishan Patel <kishan.b.patel@sas.com>
@kpatl1
Copy link
Collaborator Author

kpatl1 commented Aug 14, 2025

Thinking about it further, you can just check if the path can be copied before you show the tooltip (i.e. checking item.contextValue.includes('copyPath'))

You're totally right. Thank you for pointing these things out to me. I have gone in and added the copyPath optimization to only show valid paths. Furthermore, I have added caching for the parent to allow for the path to be concatenated for every sibling after the original server call. I am not sure if I fully implemented it correctly, so I would love if you could take a look at it when you get a chance.

@kpatl1 kpatl1 assigned kpatl1 and unassigned kpatl1 Aug 14, 2025
@scnwwu
Copy link
Contributor

scnwwu commented Aug 15, 2025

I think it's not only about the tooltip on the tree item. If you open a file from SAS Content/Server and hover on the editor tab header, you'll get the same tooltip.
image
It's more crucial here as user has no way to know where does this file come from. If there're 2 files with same name, there's no way to distinguish them.
VS Code displays the URI as tooltip by default. Ideally if we can make the uri field be a read-able path. The tooltip will be good everywhere.

@scnwwu
Copy link
Contributor

scnwwu commented Aug 15, 2025

I have the same concern with @scottdover about the network call. It's not only one call per file but n calls per file where n is the nested level, since the getPathOfItem call is recursive.
Like #1559, I think in most cases user expands the tree from root and the parents should be already known, instead of having to query back.

@scottdover
Copy link
Contributor

scottdover commented Aug 15, 2025

I think it's not only about the tooltip on the tree item. If you open a file from SAS Content/Server and hover on the editor tab header, you'll get the same tooltip. image It's more crucial here as user has no way to know where does this file come from. If there're 2 files with same name, there's no way to distinguish them. VS Code displays the URI as tooltip by default. Ideally if we can make the uri field be a read-able path. The tooltip will be good everywhere.

Ooh, I like this idea. I think this is probably the best path forward.

Kishan Patel added 4 commits August 15, 2025 14:14
I, Kishan Patel <kishan.b.patel@sas.com>, hereby add my Signed-off-by to this commit: 05fb7f2

Signed-off-by: Kishan Patel <kishan.b.patel@sas.com>
@kpatl1
Copy link
Collaborator Author

kpatl1 commented Aug 15, 2025

Made a new commit by changing the uri to contain the full path. I have also moved the caching into the RestContentAdapter. I cannot seem to get the content in SAS Server to show the full path, however, only when I view items in My Folder. Do you guys have any advice on how I could approach this?

@scottdover
Copy link
Contributor

Made a new commit by changing the uri to contain the full path. I have also moved the caching into the RestContentAdapter. I cannot seem to get the content in SAS Server to show the full path, however, only when I view items in My Folder. Do you guys have any advice on how I could approach this?

Fwiw, sas server uris have a different structure and call getSasServerUri. But maybe I'm misunderstanding what you're saying

Copy link
Collaborator Author

@kpatl1 kpatl1 left a comment

Choose a reason for hiding this comment

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

Made a new commit by changing the uri to contain the full path. I have also moved the caching into the RestContentAdapter. I cannot seem to get the content in SAS Server to show the full path, however, only when I view items in My Folder. Do you guys have any advice on how I could approach this?

Fwiw, sas server uris have a different structure and call getSasServerUri. But maybe I'm misunderstanding what you're saying

I understand. I think my confusion came from poor wording. When you hover over a file under the SAS Content folder, it does not resolve a path only the item.name. However, for files in My Folder it works, so would I need to utilize getSasServerUri for those files?

kpatl1 added 3 commits August 22, 2025 17:27
Signed-off-by: Kishan Patel <kpatel7927@gmail.com>
Signed-off-by: Kishan Patel <kpatel7927@gmail.com>
…t support

Signed-off-by: Kishan Patel <kpatel7927@gmail.com>
@scottdover
Copy link
Contributor

Looked over this a bit more. Here's what I'm noticing:

  • The tooltips start out with /Users/.... That seems like a step in the right direction
  • We're still setting tooltip in ContentDataProvider. Looking at @scnwwu 's comments up above, I don't think we want to do that. Instead, the uri should contain the full path so that the tooltip is automatically populated correctly. At the moment, hovering over the tab on an open file doesn't show the full path
  • Also, the filename statement (where you drag a file from sas content into a sas file while holding shift) generation isn't working as expected. The folder path now contains the file, like...
filename fileref filesrvc folderpath='/Users/scdove/My Folder/test.sas' filename='test.sas';

I think the best path forward is to get rid of tooltip in ContentDataProvider, and make sure the uris for both sas content (RestContentAdapter) and sas server (RestServerAdapter, ItcServerAdapter) contain the full path (or just the name in instances where the path isn't relevant/copyable/etc)

Signed-off-by: Kishan Patel <kpatel7927@gmail.com>
Signed-off-by: Kishan Patel <kpatel7927@gmail.com>
@scottdover
Copy link
Contributor

Testing these changes out, there are a couple very minor things I noticed...

  • Do we need paths for /My Favorites and /Recycle Bin. Visually, that's probably fine, but are those actual paths at all from a sas content perspective? Either way, I think it's acceptable for those to be there
  • For SAS Server > Home, I'm seeing the path as /SAS_SERVER_HOME_DIRECTORY for the default and for a custom root path. This one seems worth fixing.

Taking a look at the code shortly.

Copy link
Contributor

@scottdover scottdover left a comment

Choose a reason for hiding this comment

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

Only a couple minor things and I think this one is good to go. I'm not 100% on fully understanding the path cache code, but it seems to work alright

Signed-off-by: Kishan Patel <kpatel7927@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.

It is better to show the path information in detail

4 participants