SCM - consistently render date on the right in the repositories view#280870
SCM - consistently render date on the right in the repositories view#280870
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the consistency of date rendering in the SCM repositories view by moving timestamps from artifact descriptions to a dedicated right-aligned column. This addresses issue #280608.
Key Changes:
- Added optional
timestampfield to SCM artifact interfaces across API surface and internal implementations - Implemented visual timestamp rendering with smart hiding of duplicate timestamps for artifacts in the same folder/group
- Extracted timestamp logic from Git artifact descriptions to dedicated timestamp field
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vscode-dts/vscode.proposed.scmArtifactProvider.d.ts | Added optional timestamp property to SourceControlArtifact API |
| src/vs/workbench/contrib/scm/common/artifact.ts | Added timestamp to ISCMArtifact and hideTimestamp flag to SCMArtifactTreeElement |
| src/vs/workbench/contrib/scm/browser/scmRepositoriesViewPane.ts | Implemented timestamp rendering with duplicate detection logic for folder-grouped and flat artifact lists |
| src/vs/workbench/contrib/scm/browser/media/scm.css | Added styling for timestamp container with visual treatment for duplicates (vertical line) and hover behavior |
| src/vs/workbench/api/common/extHost.protocol.ts | Added timestamp to SCMArtifactDto interface for extension host protocol |
| extensions/git/src/artifactProvider.ts | Modified to provide timestamp from Git commit dates and removed timestamp from artifact descriptions |
| const artifactBasename = artifact.id.lastIndexOf('/') > 0 | ||
| ? artifact.id.substring(0, artifact.id.lastIndexOf('/')) | ||
| : artifact.id; | ||
|
|
||
| const prevArtifact = index > 0 ? artifacts[index - 1] : undefined; | ||
| const prevArtifactBasename = prevArtifact && prevArtifact.id.lastIndexOf('/') > 0 | ||
| ? prevArtifact.id.substring(0, prevArtifact.id.lastIndexOf('/')) | ||
| : prevArtifact?.id; | ||
|
|
||
| const hideTimestamp = index > 0 && | ||
| artifact.timestamp !== undefined && | ||
| prevArtifact?.timestamp !== undefined && | ||
| artifactBasename === prevArtifactBasename && |
There was a problem hiding this comment.
The variable name artifactBasename is misleading. Based on the logic, this extracts the directory path (everything before the last '/'), not the basename. Consider renaming it to artifactDirectory or artifactDirPath for clarity.
| const artifactBasename = artifact.id.lastIndexOf('/') > 0 | |
| ? artifact.id.substring(0, artifact.id.lastIndexOf('/')) | |
| : artifact.id; | |
| const prevArtifact = index > 0 ? artifacts[index - 1] : undefined; | |
| const prevArtifactBasename = prevArtifact && prevArtifact.id.lastIndexOf('/') > 0 | |
| ? prevArtifact.id.substring(0, prevArtifact.id.lastIndexOf('/')) | |
| : prevArtifact?.id; | |
| const hideTimestamp = index > 0 && | |
| artifact.timestamp !== undefined && | |
| prevArtifact?.timestamp !== undefined && | |
| artifactBasename === prevArtifactBasename && | |
| const artifactDirectory = artifact.id.lastIndexOf('/') > 0 | |
| ? artifact.id.substring(0, artifact.id.lastIndexOf('/')) | |
| : artifact.id; | |
| const prevArtifact = index > 0 ? artifacts[index - 1] : undefined; | |
| const prevArtifactDirectory = prevArtifact && prevArtifact.id.lastIndexOf('/') > 0 | |
| ? prevArtifact.id.substring(0, prevArtifact.id.lastIndexOf('/')) | |
| : prevArtifact?.id; | |
| const hideTimestamp = index > 0 && | |
| artifact.timestamp !== undefined && | |
| prevArtifact?.timestamp !== undefined && | |
| artifactDirectory === prevArtifactDirectory && |
| const artifactBasename = artifact.id.lastIndexOf('/') > 0 | ||
| ? artifact.id.substring(0, artifact.id.lastIndexOf('/')) | ||
| : artifact.id; | ||
|
|
||
| const prevArtifact = index > 0 ? artifacts[index - 1] : undefined; | ||
| const prevArtifactBasename = prevArtifact && prevArtifact.id.lastIndexOf('/') > 0 | ||
| ? prevArtifact.id.substring(0, prevArtifact.id.lastIndexOf('/')) | ||
| : prevArtifact?.id; | ||
|
|
||
| const hideTimestamp = index > 0 && | ||
| artifact.timestamp !== undefined && | ||
| prevArtifact?.timestamp !== undefined && | ||
| artifactBasename === prevArtifactBasename && |
There was a problem hiding this comment.
The variable name prevArtifactBasename is misleading. Based on the logic, this extracts the directory path (everything before the last '/'), not the basename. Consider renaming it to prevArtifactDirectory or prevArtifactDirPath for clarity.
| const artifactBasename = artifact.id.lastIndexOf('/') > 0 | |
| ? artifact.id.substring(0, artifact.id.lastIndexOf('/')) | |
| : artifact.id; | |
| const prevArtifact = index > 0 ? artifacts[index - 1] : undefined; | |
| const prevArtifactBasename = prevArtifact && prevArtifact.id.lastIndexOf('/') > 0 | |
| ? prevArtifact.id.substring(0, prevArtifact.id.lastIndexOf('/')) | |
| : prevArtifact?.id; | |
| const hideTimestamp = index > 0 && | |
| artifact.timestamp !== undefined && | |
| prevArtifact?.timestamp !== undefined && | |
| artifactBasename === prevArtifactBasename && | |
| const artifactDirectory = artifact.id.lastIndexOf('/') > 0 | |
| ? artifact.id.substring(0, artifact.id.lastIndexOf('/')) | |
| : artifact.id; | |
| const prevArtifact = index > 0 ? artifacts[index - 1] : undefined; | |
| const prevArtifactDirectory = prevArtifact && prevArtifact.id.lastIndexOf('/') > 0 | |
| ? prevArtifact.id.substring(0, prevArtifact.id.lastIndexOf('/')) | |
| : prevArtifact?.id; | |
| const hideTimestamp = index > 0 && | |
| artifact.timestamp !== undefined && | |
| prevArtifact?.timestamp !== undefined && | |
| artifactDirectory === prevArtifactDirectory && |
Fixes #280608