From 4ed01fadd1e225b145bda6b57e444fb28f9eb57d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20H=C3=B8jberg?= Date: Wed, 22 Oct 2025 15:24:31 -0400 Subject: [PATCH] Add support for expanding collapsed diff sections We now collapse diff sections that don't have any changes, but now, if users want more context, they can expand and see everything. --- src/UnisonShare/BranchDiff.elm | 26 +++ src/UnisonShare/BranchDiffState.elm | 27 ++- src/UnisonShare/DefinitionDiff.elm | 31 ++++ src/UnisonShare/DefinitionDiffCard.elm | 50 ++++-- .../Page/ProjectContributionChangesPage.elm | 53 +++++- .../project-contribution-changes-page.css | 10 ++ tests/UnisonShare/BranchDiffTests.elm | 164 ++++++++++++++++++ 7 files changed, 333 insertions(+), 28 deletions(-) create mode 100644 tests/UnisonShare/BranchDiffTests.elm diff --git a/src/UnisonShare/BranchDiff.elm b/src/UnisonShare/BranchDiff.elm index 8815bdaa..b222f402 100644 --- a/src/UnisonShare/BranchDiff.elm +++ b/src/UnisonShare/BranchDiff.elm @@ -28,6 +28,32 @@ type alias BranchDiff = -- HELPERS +mapChangeLines : (ChangeLine -> ChangeLine) -> BranchDiff -> BranchDiff +mapChangeLines f bd = + { bd | lines = List.map f bd.lines } + + +updateChangeLineById : (ChangeLine -> ChangeLine) -> ChangeLineId -> BranchDiff -> BranchDiff +updateChangeLineById f id bd = + let + updateIfMatched cl = + if ChangeLine.matchesId id cl then + f cl + + else + cl + + go cl = + case cl of + Namespace items -> + Namespace { items | lines = List.map go items.lines } + + _ -> + updateIfMatched cl + in + mapChangeLines go bd + + size : BranchDiff -> Int size branchDiff = branchDiff |> summary |> .numChanges diff --git a/src/UnisonShare/BranchDiffState.elm b/src/UnisonShare/BranchDiffState.elm index f70a5f97..e3d89658 100644 --- a/src/UnisonShare/BranchDiffState.elm +++ b/src/UnisonShare/BranchDiffState.elm @@ -5,6 +5,8 @@ import Http import Json.Decode as Decode exposing (Decoder) import Lib.Decode.Helpers exposing (whenFieldIs, whenPathIs) import UnisonShare.BranchDiff as BranchDiff exposing (BranchDiff) +import UnisonShare.BranchDiff.ChangeLine exposing (ChangeLine) +import UnisonShare.BranchDiff.ChangeLineId exposing (ChangeLineId) type DiffErrorCulprit @@ -41,7 +43,30 @@ type BranchDiffState --- +-- HELPERS + + +mapChangeLines : (ChangeLine -> ChangeLine) -> BranchDiffState -> BranchDiffState +mapChangeLines f bds = + case bds of + Computed bd -> + Computed (BranchDiff.mapChangeLines f bd) + + _ -> + bds + + +updateChangeLineById : (ChangeLine -> ChangeLine) -> ChangeLineId -> BranchDiffState -> BranchDiffState +updateChangeLineById f id bds = + case bds of + Computed bd -> + Computed (BranchDiff.updateChangeLineById f id bd) + + _ -> + bds + + + -- DECODE diff --git a/src/UnisonShare/DefinitionDiff.elm b/src/UnisonShare/DefinitionDiff.elm index 0905f1bb..2f58b7fb 100644 --- a/src/UnisonShare/DefinitionDiff.elm +++ b/src/UnisonShare/DefinitionDiff.elm @@ -75,6 +75,37 @@ definitionTypeToString type_ = "Type" + +-- Collapse + + +expandCollapsedDiffSection : Int -> DefinitionDiff -> DefinitionDiff +expandCollapsedDiffSection index diff = + let + expand i line = + if i == index then + case line of + Collapsed diffLines -> + NotCollapsed diffLines + + _ -> + line + + else + line + in + case diff of + Diff details -> + Diff + { details + | left = List.indexedMap expand details.left + , right = List.indexedMap expand details.right + } + + _ -> + diff + + isCollapsable : DiffLine -> Bool isCollapsable l = case l of diff --git a/src/UnisonShare/DefinitionDiffCard.elm b/src/UnisonShare/DefinitionDiffCard.elm index ed26842b..a7453ea2 100644 --- a/src/UnisonShare/DefinitionDiffCard.elm +++ b/src/UnisonShare/DefinitionDiffCard.elm @@ -7,6 +7,8 @@ import Html exposing (Html, code, div, header, pre, span, text) import Html.Attributes exposing (class, style) import List.Nonempty as NEL import String.Extra exposing (pluralize) +import UI.Click as Click +import UI.Icon as Icon import UI.Tooltip as Tooltip import UnisonShare.DefinitionDiff as DefinitionDiff exposing (DefinitionDiff(..), DiffDetails, DiffLine(..), DiffSegment(..)) @@ -113,21 +115,25 @@ viewDiffLine viewSeg changeIndicator gutterWidth line = [] -viewCollapsed : (DiffLine -> Html msg) -> DefinitionDiff.Collapsed -> Html msg -viewCollapsed viewLine collapsed = +viewCollapsed : ViewConfig msg -> (DiffLine -> Html msg) -> Int -> DefinitionDiff.Collapsed -> Html msg +viewCollapsed cfg viewLine index collapsed = case collapsed of DefinitionDiff.Collapsed lines -> let numCollapsedLines = List.length lines in - div [ class "collapsed-section" ] - [ text - (String.fromInt numCollapsedLines - ++ pluralize " line " " lines " numCollapsedLines - ++ "hidden..." - ) - ] + Click.onClick (cfg.toExpandCollapsedDiffSectionMsg { index = index }) + |> Click.view + [ class "collapsed-section" ] + [ Icon.view Icon.dots + , text + (String.fromInt numCollapsedLines + ++ pluralize "line " "lines " numCollapsedLines + ++ "hidden" + ) + , Icon.view Icon.dots + ] DefinitionDiff.NotCollapsed lines -> div [] (List.map viewLine lines) @@ -149,14 +155,14 @@ diffLength collapsedLines = |> List.sum -viewDiff : (Bool -> SyntaxConfig msg) -> DiffDetails -> Html msg -viewDiff toSyntaxConfig { left, right } = +viewDiff : ViewConfig msg -> DiffDetails -> Html msg +viewDiff cfg { left, right } = let toGutterWidth len = String.length (String.fromInt len) toViewDiffSegment isNew = - viewDiffSegment (toSyntaxConfig isNew) + viewDiffSegment (cfg.toSyntaxConfig isNew) viewLeftDiffLine = viewDiffLine (toViewDiffSegment False) @@ -169,10 +175,10 @@ viewDiff toSyntaxConfig { left, right } = (toGutterWidth (diffLength right)) before = - List.map (viewCollapsed viewLeftDiffLine) left + List.indexedMap (viewCollapsed cfg viewLeftDiffLine) left after = - List.map (viewCollapsed viewRightDiffLine) right + List.indexedMap (viewCollapsed cfg viewRightDiffLine) right in div [ class "diff-side-by-side" ] [ pre [ class "monochrome diff-side left" ] @@ -186,14 +192,20 @@ viewDiff toSyntaxConfig { left, right } = ] -view : (Bool -> SyntaxConfig msg) -> DefinitionDiff -> Html msg -view toSyntaxConfig defDiff = +type alias ViewConfig msg = + { toSyntaxConfig : Bool -> SyntaxConfig msg + , toExpandCollapsedDiffSectionMsg : { index : Int } -> msg + } + + +view : ViewConfig msg -> DefinitionDiff -> Html msg +view cfg defDiff = case defDiff of Diff details -> - div [] [ viewDiff toSyntaxConfig details ] + div [] [ viewDiff cfg details ] Mismatched { left, right } -> div [ class "diff-side-by-side" ] - [ pre [ class "monochrome diff-side" ] [ code [] (viewSegments (toSyntaxConfig False) "mismatched old" left) ] - , pre [ class "monochrome diff-side" ] [ code [] (viewSegments (toSyntaxConfig True) "mismatched new" right) ] + [ pre [ class "monochrome diff-side" ] [ code [] (viewSegments (cfg.toSyntaxConfig False) "mismatched old" left) ] + , pre [ class "monochrome diff-side" ] [ code [] (viewSegments (cfg.toSyntaxConfig True) "mismatched new" right) ] ] diff --git a/src/UnisonShare/Page/ProjectContributionChangesPage.elm b/src/UnisonShare/Page/ProjectContributionChangesPage.elm index 85f55795..bb68c74e 100644 --- a/src/UnisonShare/Page/ProjectContributionChangesPage.elm +++ b/src/UnisonShare/Page/ProjectContributionChangesPage.elm @@ -38,6 +38,7 @@ import UnisonShare.BranchDiff.ToggledChangeLines as ToggledChangeLines exposing import UnisonShare.BranchDiffState as BranchDiffState exposing (BranchDiffState) import UnisonShare.Contribution exposing (ContributionDetails) import UnisonShare.Contribution.ContributionRef exposing (ContributionRef) +import UnisonShare.DefinitionDiff as DefinitionDiff import UnisonShare.DefinitionDiffCard as DefinitionDiffCard import UnisonShare.Link as Link import UnisonShare.Project.ProjectRef exposing (ProjectRef) @@ -84,6 +85,7 @@ type Msg | CopyChangeLinePermalink ChangeLineId | SetChangeLinePermalink ChangeLineId | ScrollTo ChangeLineId + | ExpandCollapsedDiffSection ChangeLineId { index : Int } | NoOp @@ -163,8 +165,10 @@ update appContext projectRef contribRef msg model = SetChangeLinePermalink changeLineId -> let route = - changeLineId - |> Route.projectContributionChange projectRef contribRef + Route.projectContributionChange + projectRef + contribRef + changeLineId in ( model , Cmd.batch @@ -176,8 +180,10 @@ update appContext projectRef contribRef msg model = CopyChangeLinePermalink changeLineId -> let route = - changeLineId - |> Route.projectContributionChange projectRef contribRef + Route.projectContributionChange + projectRef + contribRef + changeLineId url = route @@ -192,6 +198,26 @@ update appContext projectRef contribRef msg model = ] ) + ExpandCollapsedDiffSection changeLineId { index } -> + let + expandCollapsedDiffSection changeLine = + case changeLine of + ChangeLine.Updated type_ details -> + ChangeLine.Updated + type_ + { details | diff = DefinitionDiff.expandCollapsedDiffSection index details.diff } + + _ -> + changeLine + + branchDiff = + BranchDiffState.updateChangeLineById + expandCollapsedDiffSection + changeLineId + model.branchDiff + in + ( { model | branchDiff = branchDiff }, Cmd.none ) + NoOp -> ( model, Cmd.none ) @@ -421,12 +447,23 @@ viewChangedDefinitionCard projectRef toggledChangeLines branchDiff maxBadgeLengt ( expanded, toggleIcon ) = if ToggledChangeLines.isCollapsed toggledChangeLines changeLine then - ( Nothing, Icon.arrowsFromLine ) + ( Nothing, Icon.expandDown ) else case changeLine of ChangeLine.Updated _ { diff } -> - ( Just (DefinitionDiffCard.view toSyntaxConfig diff), Icon.arrowsToLine ) + case ChangeLine.toChangeLineId changeLine of + Just id -> + let + viewConfig = + { toSyntaxConfig = toSyntaxConfig + , toExpandCollapsedDiffSectionMsg = ExpandCollapsedDiffSection id + } + in + ( Just (DefinitionDiffCard.view viewConfig diff), Icon.collapseUp ) + + Nothing -> + ( Nothing, Icon.dash ) _ -> case ChangeLine.source changeLine of @@ -451,11 +488,11 @@ viewChangedDefinitionCard projectRef toggledChangeLines branchDiff maxBadgeLengt [ code [] [ Syntax.view linked source ] ] in ( Just expandedContent - , Icon.arrowsToLine + , Icon.collapseUp ) Nothing -> - ( Nothing, Icon.arrowsFromLine ) + ( Nothing, Icon.expandDown ) domId = changeLine diff --git a/src/css/unison-share/page/project-contribution-changes-page.css b/src/css/unison-share/page/project-contribution-changes-page.css index e1ac9109..49f0e98a 100644 --- a/src/css/unison-share/page/project-contribution-changes-page.css +++ b/src/css/unison-share/page/project-contribution-changes-page.css @@ -230,6 +230,7 @@ } & .definition-change.card .collapsed-section { + border-radius: 0; color: var(--u-color_text_subdued); font-family: var(--font-monospace); background: var(--u-color_element_subdued); @@ -244,6 +245,15 @@ border-top: 1px solid var(--u-color_border_subdued); border-bottom: 1px solid var(--u-color_border_subdued); font-size: var(--font-size-extra-small); + overflow: hidden; + } + + & .definition-change.card .collapsed-section .icon { + color: var(--u-color_icon_subdued); + } + + & .definition-change.card .collapsed-section:hover { + color: var(--u-color_interactive); } & .definition-change.card .collapsed-section:first-child { diff --git a/tests/UnisonShare/BranchDiffTests.elm b/tests/UnisonShare/BranchDiffTests.elm new file mode 100644 index 00000000..cad60437 --- /dev/null +++ b/tests/UnisonShare/BranchDiffTests.elm @@ -0,0 +1,164 @@ +module UnisonShare.BranchDiffTests exposing (..) + +import Code.BranchRef as BranchRef +import Code.Definition.Reference as Reference +import Code.FullyQualifiedName as FQN +import Code.Hash as Hash +import Code.Syntax.SyntaxSegment as SyntaxSegment +import Expect +import List.Nonempty as NEL +import Test exposing (..) +import UnisonShare.BranchDiff as BranchDiff +import UnisonShare.BranchDiff.ChangeLine as ChangeLine +import UnisonShare.BranchDiff.ChangeLineId as ChangeLineId +import UnisonShare.BranchDiff.DefinitionType as DefinitionType +import UnisonShare.DefinitionDiff as DefinitionDiff + + +updateChangeLineById : Test +updateChangeLineById = + describe "BranchDiff.updateChangeLineById" + [ test "calls the updater function if the change line matches" <| + \_ -> + let + fullName = + FQN.fromString "data.List.map" + + updatedChangeLine = + changeLine fullName (FQN.fromString "List.map") + + changeLineId = + ChangeLineId.changeLineId + ChangeLineId.Updated + DefinitionType.Term + fullName + + update_ cl = + case cl of + ChangeLine.Updated dt d -> + ChangeLine.Updated dt { d | shortName = FQN.fromString "Updated" } + + _ -> + cl + + input = + branchDiff [ updatedChangeLine ] + + expected = + branchDiff [ changeLine fullName (FQN.fromString "Updated") ] + + result = + BranchDiff.updateChangeLineById update_ changeLineId input + in + Expect.equal expected result + , test "calls the updater function if the change line matches even if that change line is nested inside of Namespace" <| + \_ -> + let + fullName = + FQN.fromString "data.List.map" + + updatedChangeLine = + changeLine fullName (FQN.fromString "List.map") + + namespace = + ChangeLine.Namespace + { name = FQN.fromString "data.List" + , lines = [ updatedChangeLine ] + } + + changeLineId = + ChangeLineId.changeLineId + ChangeLineId.Updated + DefinitionType.Term + fullName + + update_ cl = + case cl of + ChangeLine.Updated dt d -> + ChangeLine.Updated dt { d | shortName = FQN.fromString "Updated" } + + _ -> + cl + + input = + branchDiff [ namespace ] + + expected = + branchDiff + [ ChangeLine.Namespace + { name = FQN.fromString "data.List" + , lines = [ changeLine fullName (FQN.fromString "Updated") ] + } + ] + + result = + BranchDiff.updateChangeLineById update_ changeLineId input + in + Expect.equal expected result + ] + + +branchDiff : List ChangeLine.ChangeLine -> BranchDiff.BranchDiff +branchDiff lines = + { lines = lines + , oldBranch = oldBranch + , newBranch = newBranch + , libDeps = [] + } + + +oldBranch : BranchDiff.DiffBranchRef +oldBranch = + { ref = BranchRef.unsafeFromString "@unison/oldBranch" + , hash = Hash.unsafeFromString "oldBranch" + } + + +newBranch : BranchDiff.DiffBranchRef +newBranch = + { ref = BranchRef.unsafeFromString "@unison/newBranch" + , hash = Hash.unsafeFromString "newbranch" + } + + +changeLine : FQN.FQN -> FQN.FQN -> ChangeLine.ChangeLine +changeLine fullName shortName = + ChangeLine.Updated + DefinitionType.Term + { oldHash = Hash.unsafeFromString "#oldHash" + , newHash = Hash.unsafeFromString "#newHash" + , shortName = shortName + , fullName = fullName + , ref = Reference.fromFQN Reference.TermReference fullName + , diff = diff + } + + +diff : DefinitionDiff.DefinitionDiff +diff = + let + diffDetails = + { type_ = DefinitionDiff.Term + , left = + [ DefinitionDiff.NotCollapsed + [ DefinitionDiff.ChangedLine + { lineNum = 1 + , segments = + [ DefinitionDiff.Both (NEL.singleton (SyntaxSegment.SyntaxSegment SyntaxSegment.TextLiteral "oldDef")) + ] + } + ] + ] + , right = + [ DefinitionDiff.NotCollapsed + [ DefinitionDiff.ChangedLine + { lineNum = 1 + , segments = + [ DefinitionDiff.Both (NEL.singleton (SyntaxSegment.SyntaxSegment SyntaxSegment.TextLiteral "newDef")) + ] + } + ] + ] + } + in + DefinitionDiff.Diff diffDetails