Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the long distance hint widget implementation to use a derived observable pattern and enhances the UI with improved layout and visual indicators.
- Wraps
_widgetContentin aderivedobservable to better integrate with the reactive architecture - Adds a "Go To Edit" label with directional arrow icon to indicate edit location relative to the hint
- Improves code organization by extracting outline rendering logic into a separate helper function
isEditBelowHint
| function isEditBelowHint(viewState: ILongDistanceViewState): boolean { | ||
| const hintLineNumber = viewState.hint.lineNumber; | ||
| const editStartLineNumber = viewState.diff[0]?.original.startLineNumber; | ||
| return hintLineNumber < editStartLineNumber; |
There was a problem hiding this comment.
The function doesn't handle the case where viewState.diff is an empty array. If viewState.diff[0] is undefined, editStartLineNumber will be undefined, causing the comparison hintLineNumber < editStartLineNumber to return false (comparing number with undefined). Consider adding a fallback or explicit check:
function isEditBelowHint(viewState: ILongDistanceViewState): boolean {
const hintLineNumber = viewState.hint.lineNumber;
const editStartLineNumber = viewState.diff[0]?.original.startLineNumber;
return editStartLineNumber !== undefined && hintLineNumber < editStartLineNumber;
}| return hintLineNumber < editStartLineNumber; | |
| return editStartLineNumber !== undefined && hintLineNumber < editStartLineNumber; |
| this._viewState.get()?.model.jump(); | ||
| } | ||
| }, [ | ||
| private readonly _widgetContent = derived(this, reader => // TODO how to not use derived but not move into constructor? |
There was a problem hiding this comment.
[nitpick] Consider resolving the TODO comment before merging. The comment asks "how to not use derived but not move into constructor?" This suggests uncertainty about the current approach. If this is the intended final implementation, the TODO should be removed or converted to a regular comment explaining why derived is necessary here.
| private readonly _widgetContent = derived(this, reader => // TODO how to not use derived but not move into constructor? | |
| // The use of `derived` here is necessary to ensure `_widgetContent` reacts to changes in its dependencies. | |
| private readonly _widgetContent = derived(this, reader => |
| const icon = SymbolKinds.toIcon(item.kind); | ||
| outlineElements.push(n.div({ | ||
| class: 'breadcrumb-item', | ||
| style: { display: 'flex', alignItems: 'center', flex: '1 1 auto', overflow: 'hidden', textOverflow: 'ellipsis', whiteSpace: 'nowrap' }, |
There was a problem hiding this comment.
should this be a class somewhere?
| const arrowIcon = isEditBelowHint(viewState) ? Codicon.arrowDown : Codicon.arrowUp; | ||
| children.push(n.div({ | ||
| class: 'go-to-label', | ||
| style: { display: 'flex', alignItems: 'center', flex: '0 0 auto', marginLeft: '14px' }, |
Copilot Generated Description:Adjust imports and refine the implementation of the long distance hint feature, including updates to widget content handling and style properties.