From 7bd81ad47a3c0e48b519f7b7199bf16a09ca5336 Mon Sep 17 00:00:00 2001 From: davidvader Date: Wed, 7 Aug 2024 09:47:39 -0500 Subject: [PATCH 1/7] fix: stop double log fetch on steps/services --- src/elm/Pages/Org_/Repo_/Build_.elm | 65 ++++++++++++-------- src/elm/Pages/Org_/Repo_/Build_/Services.elm | 65 ++++++++++++-------- 2 files changed, 80 insertions(+), 50 deletions(-) diff --git a/src/elm/Pages/Org_/Repo_/Build_.elm b/src/elm/Pages/Org_/Repo_/Build_.elm index 7ad635107..6e44c9730 100644 --- a/src/elm/Pages/Org_/Repo_/Build_.elm +++ b/src/elm/Pages/Org_/Repo_/Build_.elm @@ -235,7 +235,7 @@ type Msg | GetBuildStepLogResponse { step : Vela.Step, applyDomFocus : Bool, previousFocus : Maybe Focus.Focus } (Result (Http.Detailed.Error String) ( Http.Metadata, Vela.Log )) | GetBuildStepLogRefreshResponse { step : Vela.Step } (Result (Http.Detailed.Error String) ( Http.Metadata, Vela.Log )) | ClickStep { step : Vela.Step } - | ExpandStep { step : Vela.Step, applyDomFocus : Bool, previousFocus : Maybe Focus.Focus } + | ExpandStep { step : Vela.Step, applyDomFocus : Bool, previousFocus : Maybe Focus.Focus, fetchLog : Bool } | CollapseStep { step : Vela.Step } | ExpandAll | CollapseAll @@ -265,7 +265,7 @@ update shared route msg model = } , RemoteData.withDefault [] model.steps |> List.filter (\s -> Maybe.withDefault -1 focus.group == s.number) - |> List.map (\s -> ExpandStep { step = s, applyDomFocus = True, previousFocus = Just model.focus }) + |> List.map (\s -> ExpandStep { step = s, applyDomFocus = True, previousFocus = Just model.focus, fetchLog = True }) |> List.map Effect.sendMsg |> Effect.batch ) @@ -302,6 +302,7 @@ update shared route msg model = { step = step , applyDomFocus = options.applyDomFocus , previousFocus = Nothing + , fetchLog = True } |> Effect.sendMsg ) @@ -439,7 +440,7 @@ update shared route msg model = else Effect.batch - [ ExpandStep { step = options.step, applyDomFocus = False, previousFocus = Nothing } + [ ExpandStep { step = options.step, applyDomFocus = False, previousFocus = Nothing, fetchLog = False } |> Effect.sendMsg , case model.focus.a of Nothing -> @@ -459,25 +460,24 @@ update shared route msg model = ) ExpandStep options -> - ( { model - | viewing = List.Extra.unique <| options.step.number :: model.viewing - } - , Effect.batch - [ Effect.getBuildStepLog - { baseUrl = shared.velaAPIBaseURL - , session = shared.session - , onResponse = - GetBuildStepLogResponse - { step = options.step - , applyDomFocus = options.applyDomFocus - , previousFocus = options.previousFocus - } - , org = route.params.org - , repo = route.params.repo - , build = route.params.build - , stepNumber = String.fromInt options.step.number - } - , if options.applyDomFocus then + let + getBuildStepLogEffect = + Effect.getBuildStepLog + { baseUrl = shared.velaAPIBaseURL + , session = shared.session + , onResponse = + GetBuildStepLogResponse + { step = options.step + , applyDomFocus = options.applyDomFocus + , previousFocus = options.previousFocus + } + , org = route.params.org + , repo = route.params.repo + , build = route.params.build + , stepNumber = String.fromInt options.step.number + } + + applyDomFocusEffect = case ( model.focus.group, model.focus.a, model.focus.b ) of ( Just g, Nothing, Nothing ) -> FocusOn @@ -493,9 +493,23 @@ update shared route msg model = _ -> Effect.none - else - Effect.none - ] + runEffects = + [ if options.fetchLog then + getBuildStepLogEffect + + else + Effect.none + , if options.applyDomFocus then + applyDomFocusEffect + + else + Effect.none + ] + in + ( { model + | viewing = List.Extra.unique <| options.step.number :: model.viewing + } + , Effect.batch runEffects ) CollapseStep options -> @@ -525,6 +539,7 @@ update shared route msg model = { step = step , applyDomFocus = False , previousFocus = Nothing + , fetchLog = True } ) |> List.map Effect.sendMsg diff --git a/src/elm/Pages/Org_/Repo_/Build_/Services.elm b/src/elm/Pages/Org_/Repo_/Build_/Services.elm index cde941f06..cfef8847e 100644 --- a/src/elm/Pages/Org_/Repo_/Build_/Services.elm +++ b/src/elm/Pages/Org_/Repo_/Build_/Services.elm @@ -227,7 +227,7 @@ type Msg | GetBuildServiceLogResponse { service : Vela.Service, applyDomFocus : Bool, previousFocus : Maybe Focus.Focus } (Result (Http.Detailed.Error String) ( Http.Metadata, Vela.Log )) | GetBuildServiceLogRefreshResponse { service : Vela.Service } (Result (Http.Detailed.Error String) ( Http.Metadata, Vela.Log )) | ClickService { service : Vela.Service } - | ExpandService { service : Vela.Service, applyDomFocus : Bool, previousFocus : Maybe Focus.Focus } + | ExpandService { service : Vela.Service, applyDomFocus : Bool, previousFocus : Maybe Focus.Focus, fetchLog : Bool } | CollapseService { service : Vela.Service } | ExpandAll | CollapseAll @@ -257,7 +257,7 @@ update shared route msg model = } , RemoteData.withDefault [] model.services |> List.filter (\s -> Maybe.withDefault -1 focus.group == s.number) - |> List.map (\s -> ExpandService { service = s, applyDomFocus = True, previousFocus = Just model.focus }) + |> List.map (\s -> ExpandService { service = s, applyDomFocus = True, previousFocus = Just model.focus, fetchLog = True }) |> List.map Effect.sendMsg |> Effect.batch ) @@ -293,6 +293,7 @@ update shared route msg model = { service = service , applyDomFocus = True , previousFocus = Nothing + , fetchLog = False } |> Effect.sendMsg ) @@ -430,7 +431,7 @@ update shared route msg model = else Effect.batch - [ ExpandService { service = options.service, applyDomFocus = False, previousFocus = Nothing } + [ ExpandService { service = options.service, applyDomFocus = False, previousFocus = Nothing, fetchLog = False } |> Effect.sendMsg , case model.focus.a of Nothing -> @@ -450,25 +451,24 @@ update shared route msg model = ) ExpandService options -> - ( { model - | viewing = List.Extra.unique <| options.service.number :: model.viewing - } - , Effect.batch - [ Effect.getBuildServiceLog - { baseUrl = shared.velaAPIBaseURL - , session = shared.session - , onResponse = - GetBuildServiceLogResponse - { service = options.service - , applyDomFocus = options.applyDomFocus - , previousFocus = options.previousFocus - } - , org = route.params.org - , repo = route.params.repo - , build = route.params.build - , serviceNumber = String.fromInt options.service.number - } - , if options.applyDomFocus then + let + getBuildServiceLogEffect = + Effect.getBuildServiceLog + { baseUrl = shared.velaAPIBaseURL + , session = shared.session + , onResponse = + GetBuildServiceLogResponse + { service = options.service + , applyDomFocus = options.applyDomFocus + , previousFocus = options.previousFocus + } + , org = route.params.org + , repo = route.params.repo + , build = route.params.build + , serviceNumber = String.fromInt options.service.number + } + + applyDomFocusEffect = case ( model.focus.group, model.focus.a, model.focus.b ) of ( Just g, Nothing, Nothing ) -> FocusOn @@ -484,9 +484,23 @@ update shared route msg model = _ -> Effect.none - else - Effect.none - ] + runEffects = + [ if options.fetchLog then + getBuildServiceLogEffect + + else + Effect.none + , if options.applyDomFocus then + applyDomFocusEffect + + else + Effect.none + ] + in + ( { model + | viewing = List.Extra.unique <| options.service.number :: model.viewing + } + , Effect.batch runEffects ) CollapseService options -> @@ -516,6 +530,7 @@ update shared route msg model = { service = service , applyDomFocus = False , previousFocus = Nothing + , fetchLog = True } ) |> List.map Effect.sendMsg From 7babb25784b66e9d0d93a97d37dca712e4065d81 Mon Sep 17 00:00:00 2001 From: davidvader Date: Wed, 7 Aug 2024 10:20:18 -0500 Subject: [PATCH 2/7] fix: ensure logs are fetched on reclick --- src/elm/Pages/Org_/Repo_/Build_.elm | 4 ++-- src/elm/Pages/Org_/Repo_/Build_/Services.elm | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/elm/Pages/Org_/Repo_/Build_.elm b/src/elm/Pages/Org_/Repo_/Build_.elm index 6e44c9730..1db4eb812 100644 --- a/src/elm/Pages/Org_/Repo_/Build_.elm +++ b/src/elm/Pages/Org_/Repo_/Build_.elm @@ -265,7 +265,7 @@ update shared route msg model = } , RemoteData.withDefault [] model.steps |> List.filter (\s -> Maybe.withDefault -1 focus.group == s.number) - |> List.map (\s -> ExpandStep { step = s, applyDomFocus = True, previousFocus = Just model.focus, fetchLog = True }) + |> List.map (\s -> ExpandStep { step = s, applyDomFocus = True, previousFocus = Just model.focus, fetchLog = model.focus /= focus }) |> List.map Effect.sendMsg |> Effect.batch ) @@ -440,7 +440,7 @@ update shared route msg model = else Effect.batch - [ ExpandStep { step = options.step, applyDomFocus = False, previousFocus = Nothing, fetchLog = False } + [ ExpandStep { step = options.step, applyDomFocus = False, previousFocus = Nothing, fetchLog = model.focus.group == Just options.step.number } |> Effect.sendMsg , case model.focus.a of Nothing -> diff --git a/src/elm/Pages/Org_/Repo_/Build_/Services.elm b/src/elm/Pages/Org_/Repo_/Build_/Services.elm index cfef8847e..3c7aa82ce 100644 --- a/src/elm/Pages/Org_/Repo_/Build_/Services.elm +++ b/src/elm/Pages/Org_/Repo_/Build_/Services.elm @@ -257,7 +257,7 @@ update shared route msg model = } , RemoteData.withDefault [] model.services |> List.filter (\s -> Maybe.withDefault -1 focus.group == s.number) - |> List.map (\s -> ExpandService { service = s, applyDomFocus = True, previousFocus = Just model.focus, fetchLog = True }) + |> List.map (\s -> ExpandService { service = s, applyDomFocus = True, previousFocus = Just model.focus, fetchLog = model.focus /= focus }) |> List.map Effect.sendMsg |> Effect.batch ) @@ -431,7 +431,7 @@ update shared route msg model = else Effect.batch - [ ExpandService { service = options.service, applyDomFocus = False, previousFocus = Nothing, fetchLog = False } + [ ExpandService { service = options.service, applyDomFocus = False, previousFocus = Nothing, fetchLog = model.focus.group == Just options.service.number } |> Effect.sendMsg , case model.focus.a of Nothing -> From 74ee9183c76fe94180b45e203d5a8dce4d7a7694 Mon Sep 17 00:00:00 2001 From: davidvader Date: Wed, 7 Aug 2024 10:44:10 -0500 Subject: [PATCH 3/7] chore: comments --- src/elm/Pages/Org_/Repo_/Build_.elm | 27 ++++++++++++++++++-- src/elm/Pages/Org_/Repo_/Build_/Services.elm | 26 +++++++++++++++++-- 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/src/elm/Pages/Org_/Repo_/Build_.elm b/src/elm/Pages/Org_/Repo_/Build_.elm index 1db4eb812..e92fa6aa7 100644 --- a/src/elm/Pages/Org_/Repo_/Build_.elm +++ b/src/elm/Pages/Org_/Repo_/Build_.elm @@ -256,6 +256,7 @@ update shared route msg model = -- BROWSER OnHashChanged options -> + -- fixme: this is fetching a new build when you click log lines XD let focus = Focus.fromString options.to @@ -265,7 +266,18 @@ update shared route msg model = } , RemoteData.withDefault [] model.steps |> List.filter (\s -> Maybe.withDefault -1 focus.group == s.number) - |> List.map (\s -> ExpandStep { step = s, applyDomFocus = True, previousFocus = Just model.focus, fetchLog = model.focus /= focus }) + |> List.map + (\s -> + ExpandStep + { step = s + , applyDomFocus = True + , previousFocus = Just model.focus + + -- prevent double fetching logs when hash changed but the group is the same + -- ex, #2:1 -> #2:2 + , fetchLog = model.focus.group /= focus.group + } + ) |> List.map Effect.sendMsg |> Effect.batch ) @@ -302,6 +314,8 @@ update shared route msg model = { step = step , applyDomFocus = options.applyDomFocus , previousFocus = Nothing + + -- no reason to fetch logs in this scenario , fetchLog = True } |> Effect.sendMsg @@ -440,7 +454,14 @@ update shared route msg model = else Effect.batch - [ ExpandStep { step = options.step, applyDomFocus = False, previousFocus = Nothing, fetchLog = model.focus.group == Just options.step.number } + [ ExpandStep + { step = options.step + , applyDomFocus = False + , previousFocus = Nothing + + -- OnHashChanged handles fetching logs when the focus has changed + , fetchLog = model.focus.group == Just options.step.number + } |> Effect.sendMsg , case model.focus.a of Nothing -> @@ -539,6 +560,8 @@ update shared route msg model = { step = step , applyDomFocus = False , previousFocus = Nothing + + -- OnHashChanged will not fetch logs in this scenario , fetchLog = True } ) diff --git a/src/elm/Pages/Org_/Repo_/Build_/Services.elm b/src/elm/Pages/Org_/Repo_/Build_/Services.elm index 3c7aa82ce..c4589056f 100644 --- a/src/elm/Pages/Org_/Repo_/Build_/Services.elm +++ b/src/elm/Pages/Org_/Repo_/Build_/Services.elm @@ -257,7 +257,18 @@ update shared route msg model = } , RemoteData.withDefault [] model.services |> List.filter (\s -> Maybe.withDefault -1 focus.group == s.number) - |> List.map (\s -> ExpandService { service = s, applyDomFocus = True, previousFocus = Just model.focus, fetchLog = model.focus /= focus }) + |> List.map + (\s -> + ExpandService + { service = s + , applyDomFocus = True + , previousFocus = Just model.focus + + -- prevent double fetching logs when hash changed but the group is the same + -- ex, #2:1 -> #2:2 + , fetchLog = model.focus /= focus + } + ) |> List.map Effect.sendMsg |> Effect.batch ) @@ -293,6 +304,8 @@ update shared route msg model = { service = service , applyDomFocus = True , previousFocus = Nothing + + -- no reason to fetch logs in this scenario , fetchLog = False } |> Effect.sendMsg @@ -431,7 +444,14 @@ update shared route msg model = else Effect.batch - [ ExpandService { service = options.service, applyDomFocus = False, previousFocus = Nothing, fetchLog = model.focus.group == Just options.service.number } + [ ExpandService + { service = options.service + , applyDomFocus = False + , previousFocus = Nothing + + -- OnHashChanged handles fetching logs when the focus has changed + , fetchLog = model.focus.group == Just options.service.number + } |> Effect.sendMsg , case model.focus.a of Nothing -> @@ -530,6 +550,8 @@ update shared route msg model = { service = service , applyDomFocus = False , previousFocus = Nothing + + -- OnHashChanged will not fetch logs in this scenario , fetchLog = True } ) From ec60d6c03050d7e21e10e6f8a54163284dff2335 Mon Sep 17 00:00:00 2001 From: davidvader Date: Wed, 7 Aug 2024 11:38:07 -0500 Subject: [PATCH 4/7] fix: use hash focus logic to control log fetching --- src/elm/Pages/Org_/Repo_/Build_.elm | 75 +++++++++++++------ src/elm/Pages/Org_/Repo_/Build_/Services.elm | 76 ++++++++++++++------ src/elm/Utils/Helpers.elm | 15 ++-- 3 files changed, 116 insertions(+), 50 deletions(-) diff --git a/src/elm/Pages/Org_/Repo_/Build_.elm b/src/elm/Pages/Org_/Repo_/Build_.elm index 7ad635107..37dc4966b 100644 --- a/src/elm/Pages/Org_/Repo_/Build_.elm +++ b/src/elm/Pages/Org_/Repo_/Build_.elm @@ -459,25 +459,40 @@ update shared route msg model = ) ExpandStep options -> - ( { model - | viewing = List.Extra.unique <| options.step.number :: model.viewing - } - , Effect.batch - [ Effect.getBuildStepLog - { baseUrl = shared.velaAPIBaseURL - , session = shared.session - , onResponse = - GetBuildStepLogResponse - { step = options.step - , applyDomFocus = options.applyDomFocus - , previousFocus = options.previousFocus - } - , org = route.params.org - , repo = route.params.repo - , build = route.params.build - , stepNumber = String.fromInt options.step.number - } - , if options.applyDomFocus then + let + isFromHashChanged = + options.previousFocus /= Nothing + + focusChanged = + case options.previousFocus of + Just f -> + f.group /= model.focus.group + + Nothing -> + False + + isLogLoaded = + Dict.get options.step.id model.logs + |> Maybe.withDefault RemoteData.Loading + |> Util.isLoaded + + getLogEffect = + Effect.getBuildStepLog + { baseUrl = shared.velaAPIBaseURL + , session = shared.session + , onResponse = + GetBuildStepLogResponse + { step = options.step + , applyDomFocus = options.applyDomFocus + , previousFocus = options.previousFocus + } + , org = route.params.org + , repo = route.params.repo + , build = route.params.build + , stepNumber = String.fromInt options.step.number + } + + applyDomFocusEffect = case ( model.focus.group, model.focus.a, model.focus.b ) of ( Just g, Nothing, Nothing ) -> FocusOn @@ -493,9 +508,25 @@ update shared route msg model = _ -> Effect.none - else - Effect.none - ] + runEffects = + [ -- fetch logs when the resource is expanded via header click + -- OR when the hash changes and the requested log is not loaded + if (focusChanged && not isLogLoaded) || not isFromHashChanged then + getLogEffect + + else + Effect.none + , if options.applyDomFocus then + applyDomFocusEffect + + else + Effect.none + ] + in + ( { model + | viewing = List.Extra.unique <| options.step.number :: model.viewing + } + , Effect.batch runEffects ) CollapseStep options -> diff --git a/src/elm/Pages/Org_/Repo_/Build_/Services.elm b/src/elm/Pages/Org_/Repo_/Build_/Services.elm index cde941f06..e25403a87 100644 --- a/src/elm/Pages/Org_/Repo_/Build_/Services.elm +++ b/src/elm/Pages/Org_/Repo_/Build_/Services.elm @@ -450,25 +450,40 @@ update shared route msg model = ) ExpandService options -> - ( { model - | viewing = List.Extra.unique <| options.service.number :: model.viewing - } - , Effect.batch - [ Effect.getBuildServiceLog - { baseUrl = shared.velaAPIBaseURL - , session = shared.session - , onResponse = - GetBuildServiceLogResponse - { service = options.service - , applyDomFocus = options.applyDomFocus - , previousFocus = options.previousFocus - } - , org = route.params.org - , repo = route.params.repo - , build = route.params.build - , serviceNumber = String.fromInt options.service.number - } - , if options.applyDomFocus then + let + isFromHashChanged = + options.previousFocus /= Nothing + + focusChanged = + case options.previousFocus of + Just f -> + f.group /= model.focus.group + + Nothing -> + False + + isLogLoaded = + Dict.get options.service.id model.logs + |> Maybe.withDefault RemoteData.Loading + |> Util.isLoaded + + getLogEffect = + Effect.getBuildServiceLog + { baseUrl = shared.velaAPIBaseURL + , session = shared.session + , onResponse = + GetBuildServiceLogResponse + { service = options.service + , applyDomFocus = options.applyDomFocus + , previousFocus = options.previousFocus + } + , org = route.params.org + , repo = route.params.repo + , build = route.params.build + , serviceNumber = String.fromInt options.service.number + } + + applyDomFocusEffect = case ( model.focus.group, model.focus.a, model.focus.b ) of ( Just g, Nothing, Nothing ) -> FocusOn @@ -484,9 +499,26 @@ update shared route msg model = _ -> Effect.none - else - Effect.none - ] + runEffects = + [ -- fetch logs when the resource is expanded via header click + -- OR when the hash changes and the requested log is not loaded + if (focusChanged && not isLogLoaded) || not isFromHashChanged then + getLogEffect + + else + Effect.none + , if options.applyDomFocus then + applyDomFocusEffect + + else + Effect.none + ] + in + ( { model + | viewing = List.Extra.unique <| options.service.number :: model.viewing + } + , Effect.batch + runEffects ) CollapseService options -> diff --git a/src/elm/Utils/Helpers.elm b/src/elm/Utils/Helpers.elm index 470bf98ff..6b3b8f532 100644 --- a/src/elm/Utils/Helpers.elm +++ b/src/elm/Utils/Helpers.elm @@ -26,7 +26,7 @@ module Utils.Helpers exposing , humanReadableDateTimeFormatter , humanReadableDateTimeWithDefault , humanReadableDateWithDefault - , isLoading + , isLoaded , isSuccess , mergeListsById , noBlanks @@ -375,17 +375,20 @@ fiveSecondsMillis = oneSecondMillis * 5 -{-| isLoading : takes WebData and returns true if it is in a Loading state. +{-| isLoaded : takes WebData and returns true if it is in a 'loaded' state, meaning its anything but NotAsked or Loading. -} -isLoading : WebData a -> Bool -isLoading status = +isLoaded : WebData a -> Bool +isLoaded status = case status of RemoteData.Loading -> - True + False - _ -> + RemoteData.NotAsked -> False + _ -> + True + {-| isSuccess : takes WebData and returns true if it is in a Success state. -} From 0090ba331e0b481d408e8542b6ff3ace8aaf4df8 Mon Sep 17 00:00:00 2001 From: davidvader Date: Wed, 7 Aug 2024 11:41:27 -0500 Subject: [PATCH 5/7] chore: cleanup --- src/elm/Pages/Org_/Repo_/Build_.elm | 11 ++++------- src/elm/Pages/Org_/Repo_/Build_/Services.elm | 11 ++++------- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/src/elm/Pages/Org_/Repo_/Build_.elm b/src/elm/Pages/Org_/Repo_/Build_.elm index 37dc4966b..88bfff007 100644 --- a/src/elm/Pages/Org_/Repo_/Build_.elm +++ b/src/elm/Pages/Org_/Repo_/Build_.elm @@ -460,16 +460,13 @@ update shared route msg model = ExpandStep options -> let - isFromHashChanged = - options.previousFocus /= Nothing - - focusChanged = + ( isFromHashChanged, didFocusChange ) = case options.previousFocus of Just f -> - f.group /= model.focus.group + ( True, f.group /= model.focus.group ) Nothing -> - False + ( False, False ) isLogLoaded = Dict.get options.step.id model.logs @@ -511,7 +508,7 @@ update shared route msg model = runEffects = [ -- fetch logs when the resource is expanded via header click -- OR when the hash changes and the requested log is not loaded - if (focusChanged && not isLogLoaded) || not isFromHashChanged then + if (didFocusChange && not isLogLoaded) || not isFromHashChanged then getLogEffect else diff --git a/src/elm/Pages/Org_/Repo_/Build_/Services.elm b/src/elm/Pages/Org_/Repo_/Build_/Services.elm index e25403a87..bdee5b5ba 100644 --- a/src/elm/Pages/Org_/Repo_/Build_/Services.elm +++ b/src/elm/Pages/Org_/Repo_/Build_/Services.elm @@ -451,16 +451,13 @@ update shared route msg model = ExpandService options -> let - isFromHashChanged = - options.previousFocus /= Nothing - - focusChanged = + ( isFromHashChanged, didFocusChange ) = case options.previousFocus of Just f -> - f.group /= model.focus.group + ( True, f.group /= model.focus.group ) Nothing -> - False + ( False, False ) isLogLoaded = Dict.get options.service.id model.logs @@ -502,7 +499,7 @@ update shared route msg model = runEffects = [ -- fetch logs when the resource is expanded via header click -- OR when the hash changes and the requested log is not loaded - if (focusChanged && not isLogLoaded) || not isFromHashChanged then + if (didFocusChange && not isLogLoaded) || not isFromHashChanged then getLogEffect else From 5a81412231aa5e12eae34316f3e89686011f5192 Mon Sep 17 00:00:00 2001 From: davidvader Date: Wed, 7 Aug 2024 13:03:37 -0500 Subject: [PATCH 6/7] fix: track trigger source for Expand and skip logs when redundant --- src/elm/Pages/Org_/Repo_/Build_.elm | 51 ++++++++++++++++---- src/elm/Pages/Org_/Repo_/Build_/Services.elm | 51 ++++++++++++++++---- 2 files changed, 84 insertions(+), 18 deletions(-) diff --git a/src/elm/Pages/Org_/Repo_/Build_.elm b/src/elm/Pages/Org_/Repo_/Build_.elm index 88bfff007..ff78134f1 100644 --- a/src/elm/Pages/Org_/Repo_/Build_.elm +++ b/src/elm/Pages/Org_/Repo_/Build_.elm @@ -235,7 +235,7 @@ type Msg | GetBuildStepLogResponse { step : Vela.Step, applyDomFocus : Bool, previousFocus : Maybe Focus.Focus } (Result (Http.Detailed.Error String) ( Http.Metadata, Vela.Log )) | GetBuildStepLogRefreshResponse { step : Vela.Step } (Result (Http.Detailed.Error String) ( Http.Metadata, Vela.Log )) | ClickStep { step : Vela.Step } - | ExpandStep { step : Vela.Step, applyDomFocus : Bool, previousFocus : Maybe Focus.Focus } + | ExpandStep { step : Vela.Step, applyDomFocus : Bool, previousFocus : Maybe Focus.Focus, triggeredFromClick : Bool } | CollapseStep { step : Vela.Step } | ExpandAll | CollapseAll @@ -265,7 +265,15 @@ update shared route msg model = } , RemoteData.withDefault [] model.steps |> List.filter (\s -> Maybe.withDefault -1 focus.group == s.number) - |> List.map (\s -> ExpandStep { step = s, applyDomFocus = True, previousFocus = Just model.focus }) + |> List.map + (\s -> + ExpandStep + { step = s + , applyDomFocus = True + , previousFocus = Just model.focus + , triggeredFromClick = False + } + ) |> List.map Effect.sendMsg |> Effect.batch ) @@ -302,6 +310,7 @@ update shared route msg model = { step = step , applyDomFocus = options.applyDomFocus , previousFocus = Nothing + , triggeredFromClick = False } |> Effect.sendMsg ) @@ -439,7 +448,12 @@ update shared route msg model = else Effect.batch - [ ExpandStep { step = options.step, applyDomFocus = False, previousFocus = Nothing } + [ ExpandStep + { step = options.step + , applyDomFocus = False + , previousFocus = Nothing + , triggeredFromClick = True + } |> Effect.sendMsg , case model.focus.a of Nothing -> @@ -460,19 +474,39 @@ update shared route msg model = ExpandStep options -> let - ( isFromHashChanged, didFocusChange ) = + isFromHashChanged = + options.previousFocus /= Nothing + + didFocusChange = case options.previousFocus of Just f -> - ( True, f.group /= model.focus.group ) + f.group /= model.focus.group Nothing -> - ( False, False ) + False + + -- hash will change when no line is selected and the selected group changes + -- this means expansion msg will double up on fetching logs unless instructed not to + willHashChange = + case ( model.focus.group, model.focus.a, model.focus.b ) of + ( Just g, Nothing, _ ) -> + g /= options.step.number + + _ -> + False isLogLoaded = Dict.get options.step.id model.logs |> Maybe.withDefault RemoteData.Loading |> Util.isLoaded + -- fetch logs when expansion msg meets the criteria: + -- triggered by a click that will change the hash + -- the focus changes and the logs are not loaded + fetchLogs = + not (options.triggeredFromClick && willHashChange) + && ((didFocusChange && not isLogLoaded) || not isFromHashChanged) + getLogEffect = Effect.getBuildStepLog { baseUrl = shared.velaAPIBaseURL @@ -506,9 +540,7 @@ update shared route msg model = Effect.none runEffects = - [ -- fetch logs when the resource is expanded via header click - -- OR when the hash changes and the requested log is not loaded - if (didFocusChange && not isLogLoaded) || not isFromHashChanged then + [ if fetchLogs then getLogEffect else @@ -553,6 +585,7 @@ update shared route msg model = { step = step , applyDomFocus = False , previousFocus = Nothing + , triggeredFromClick = False } ) |> List.map Effect.sendMsg diff --git a/src/elm/Pages/Org_/Repo_/Build_/Services.elm b/src/elm/Pages/Org_/Repo_/Build_/Services.elm index bdee5b5ba..122261a74 100644 --- a/src/elm/Pages/Org_/Repo_/Build_/Services.elm +++ b/src/elm/Pages/Org_/Repo_/Build_/Services.elm @@ -227,7 +227,7 @@ type Msg | GetBuildServiceLogResponse { service : Vela.Service, applyDomFocus : Bool, previousFocus : Maybe Focus.Focus } (Result (Http.Detailed.Error String) ( Http.Metadata, Vela.Log )) | GetBuildServiceLogRefreshResponse { service : Vela.Service } (Result (Http.Detailed.Error String) ( Http.Metadata, Vela.Log )) | ClickService { service : Vela.Service } - | ExpandService { service : Vela.Service, applyDomFocus : Bool, previousFocus : Maybe Focus.Focus } + | ExpandService { service : Vela.Service, applyDomFocus : Bool, previousFocus : Maybe Focus.Focus, triggeredFromClick : Bool } | CollapseService { service : Vela.Service } | ExpandAll | CollapseAll @@ -257,7 +257,15 @@ update shared route msg model = } , RemoteData.withDefault [] model.services |> List.filter (\s -> Maybe.withDefault -1 focus.group == s.number) - |> List.map (\s -> ExpandService { service = s, applyDomFocus = True, previousFocus = Just model.focus }) + |> List.map + (\s -> + ExpandService + { service = s + , applyDomFocus = True + , previousFocus = Just model.focus + , triggeredFromClick = False + } + ) |> List.map Effect.sendMsg |> Effect.batch ) @@ -293,6 +301,7 @@ update shared route msg model = { service = service , applyDomFocus = True , previousFocus = Nothing + , triggeredFromClick = False } |> Effect.sendMsg ) @@ -430,7 +439,12 @@ update shared route msg model = else Effect.batch - [ ExpandService { service = options.service, applyDomFocus = False, previousFocus = Nothing } + [ ExpandService + { service = options.service + , applyDomFocus = False + , previousFocus = Nothing + , triggeredFromClick = True + } |> Effect.sendMsg , case model.focus.a of Nothing -> @@ -451,19 +465,39 @@ update shared route msg model = ExpandService options -> let - ( isFromHashChanged, didFocusChange ) = + isFromHashChanged = + options.previousFocus /= Nothing + + didFocusChange = case options.previousFocus of Just f -> - ( True, f.group /= model.focus.group ) + f.group /= model.focus.group Nothing -> - ( False, False ) + False + + -- hash will change when no line is selected and the selected group changes + -- this means the expansion msg will double up on fetching logs unless instructed not to + willHashChange = + case ( model.focus.group, model.focus.a, model.focus.b ) of + ( Just g, Nothing, _ ) -> + g /= options.service.number + + _ -> + False isLogLoaded = Dict.get options.service.id model.logs |> Maybe.withDefault RemoteData.Loading |> Util.isLoaded + -- fetch logs when expansion msg meets the criteria: + -- triggered by a click that will change the hash + -- the focus changes and the logs are not loaded + fetchLogs = + not (options.triggeredFromClick && willHashChange) + && ((didFocusChange && not isLogLoaded) || not isFromHashChanged) + getLogEffect = Effect.getBuildServiceLog { baseUrl = shared.velaAPIBaseURL @@ -497,9 +531,7 @@ update shared route msg model = Effect.none runEffects = - [ -- fetch logs when the resource is expanded via header click - -- OR when the hash changes and the requested log is not loaded - if (didFocusChange && not isLogLoaded) || not isFromHashChanged then + [ if fetchLogs then getLogEffect else @@ -545,6 +577,7 @@ update shared route msg model = { service = service , applyDomFocus = False , previousFocus = Nothing + , triggeredFromClick = False } ) |> List.map Effect.sendMsg From b565089caa9d301699d343835e940e9fe2d9329d Mon Sep 17 00:00:00 2001 From: davidvader Date: Wed, 7 Aug 2024 13:05:32 -0500 Subject: [PATCH 7/7] chore: var naming --- src/elm/Pages/Org_/Repo_/Build_.elm | 4 ++-- src/elm/Pages/Org_/Repo_/Build_/Services.elm | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/elm/Pages/Org_/Repo_/Build_.elm b/src/elm/Pages/Org_/Repo_/Build_.elm index ff78134f1..91b751220 100644 --- a/src/elm/Pages/Org_/Repo_/Build_.elm +++ b/src/elm/Pages/Org_/Repo_/Build_.elm @@ -487,7 +487,7 @@ update shared route msg model = -- hash will change when no line is selected and the selected group changes -- this means expansion msg will double up on fetching logs unless instructed not to - willHashChange = + willFocusChange = case ( model.focus.group, model.focus.a, model.focus.b ) of ( Just g, Nothing, _ ) -> g /= options.step.number @@ -504,7 +504,7 @@ update shared route msg model = -- triggered by a click that will change the hash -- the focus changes and the logs are not loaded fetchLogs = - not (options.triggeredFromClick && willHashChange) + not (options.triggeredFromClick && willFocusChange) && ((didFocusChange && not isLogLoaded) || not isFromHashChanged) getLogEffect = diff --git a/src/elm/Pages/Org_/Repo_/Build_/Services.elm b/src/elm/Pages/Org_/Repo_/Build_/Services.elm index 122261a74..67249ab72 100644 --- a/src/elm/Pages/Org_/Repo_/Build_/Services.elm +++ b/src/elm/Pages/Org_/Repo_/Build_/Services.elm @@ -478,7 +478,7 @@ update shared route msg model = -- hash will change when no line is selected and the selected group changes -- this means the expansion msg will double up on fetching logs unless instructed not to - willHashChange = + willFocusChange = case ( model.focus.group, model.focus.a, model.focus.b ) of ( Just g, Nothing, _ ) -> g /= options.service.number @@ -495,7 +495,7 @@ update shared route msg model = -- triggered by a click that will change the hash -- the focus changes and the logs are not loaded fetchLogs = - not (options.triggeredFromClick && willHashChange) + not (options.triggeredFromClick && willFocusChange) && ((didFocusChange && not isLogLoaded) || not isFromHashChanged) getLogEffect =