diff --git a/review/rules/module-cohesion.md b/review/rules/module-cohesion.md new file mode 100644 index 0000000..461235f --- /dev/null +++ b/review/rules/module-cohesion.md @@ -0,0 +1,19 @@ +--- +autofix: false +model: sonnet +applies-to: "**/*.fs" +--- +# Module Cohesion + +## Rule +Code should live in the module where it logically belongs, not wherever it was first needed. + +## Why +When shared utilities accumulate in unrelated modules, the module name becomes misleading and the codebase harder to navigate. A function used across multiple modules belongs in a shared module named after what it does, not after the first consumer. + +## Requirements +- General-purpose helpers (formatting, UI components, utilities) should live in appropriately named shared modules +- A module named after a feature (e.g. `ArchiveViews`) should only contain code specific to that feature +- If a function is imported by multiple unrelated modules, it likely belongs in a shared module +- When adding a new function, check whether the target module's name accurately describes the function's purpose +- Don't create "Utils" or "Helpers" dumping grounds either — group by concept (e.g. `Components` for shared UI, `Formatting` for display helpers) diff --git a/src/Client/App.fs b/src/Client/App.fs index 58b539a..caf69fc 100644 --- a/src/Client/App.fs +++ b/src/Client/App.fs @@ -535,7 +535,7 @@ let appSubscriptions (model: Model) : Sub = else subs -let relativeTime = ArchiveViews.relativeTime +let relativeTime = Components.relativeTime let ctClassName = function @@ -1105,7 +1105,9 @@ let prRow dispatch (cooldowns: Set) (wt: WorktreeStatus) (repoName prop.children [ prBadgeContent dispatch cooldowns wt repoName pr ] ] -let workMetricsView = ArchiveViews.workMetricsView +let workMetricsView = Components.workMetricsView +let workMetricsItems = Components.workMetricsItems +let FitOrHide = Components.FitOrHide let compactWorktreeCard dispatch editorName (repoName: string) (cooldowns: Set) (scopedKey: string) (isFocused: bool) (wt: WorktreeStatus) = let baseClass = cardClassName wt + " compact" @@ -1118,9 +1120,14 @@ let compactWorktreeCard dispatch editorName (repoName: string) (cooldowns: Set) msg : UpdateResult * Cmd = | OpCompleted (Error _) -> { RefreshWorktrees = false }, Cmd.none -let relativeTime (now: System.DateTimeOffset) (dt: System.DateTimeOffset) = - let diff = now - dt - match diff with - | d when d.TotalMinutes < 1.0 -> "just now" - | d when d.TotalMinutes < 60.0 -> $"{int d.TotalMinutes}m ago" - | d when d.TotalHours < 24.0 -> $"{int d.TotalHours}h ago" - | d -> $"{int d.TotalDays}d ago" - -let workMetricsView (metrics: WorkMetrics option) = - match metrics with - | None -> Html.none - | Some m when m.CommitCount = 0 -> Html.none - | Some m -> - let displayCount = min m.CommitCount 90 - let overflow = m.CommitCount - displayCount - Html.span [ - prop.className "work-metrics" - prop.children [ - Html.span [ - prop.className "commit-grid" - prop.children ( - List.init displayCount (fun _ -> - Html.span [ prop.className "commit-square" ]) - ) - ] - if overflow > 0 then - Html.span [ prop.className "commit-overflow"; prop.text $"+{overflow}" ] - match m.LinesAdded, m.LinesRemoved with - | 0, 0 -> Html.none - | added, removed -> - Html.span [ - prop.className "diff-stats" - prop.children [ - Html.span [ prop.className "diff-added"; prop.text $"+{added}" ] - Html.text " " - Html.span [ prop.className "diff-removed"; prop.text $"-{removed}" ] - ] - ] - ] - ] - let archiveIcon = Svg.svg [ svg.className "btn-icon" diff --git a/src/Client/Client.fsproj b/src/Client/Client.fsproj index 0f3c560..acb315f 100644 --- a/src/Client/Client.fsproj +++ b/src/Client/Client.fsproj @@ -6,6 +6,7 @@ + diff --git a/src/Client/Components.fs b/src/Client/Components.fs new file mode 100644 index 0000000..8a21e29 --- /dev/null +++ b/src/Client/Components.fs @@ -0,0 +1,122 @@ +module Components + +open Shared +open Feliz +open Fable.Core +open Fable.Core.JsInterop + +let relativeTime (now: System.DateTimeOffset) (dt: System.DateTimeOffset) = + let diff = now - dt + match diff with + | d when d.TotalMinutes < 1.0 -> "just now" + | d when d.TotalMinutes < 60.0 -> $"{int d.TotalMinutes}m ago" + | d when d.TotalHours < 24.0 -> $"{int d.TotalHours}h ago" + | d -> $"{int d.TotalDays}d ago" + +// ResizeObserver interop +[] +let private createResizeObserver (callback: obj -> unit) : obj = jsNative + +[] +let private observeElement (observer: obj) (el: Browser.Types.Element) : unit = jsNative + +[] +let private disconnectObserver (observer: obj) : unit = jsNative + +[ $0.clientWidth")>] +let private hasOverflow (el: Browser.Types.Element) : bool = jsNative + +[] +let private childAt (el: Browser.Types.Element) (i: int) : Browser.Types.Element = jsNative + +[] +let private childCount (el: Browser.Types.Element) : int = jsNative + +/// Wrapper that progressively hides children when they cause the parent to overflow. +/// Items are hidden from first (lowest priority) to last (highest priority). +/// Parent must have overflow:hidden for scrollWidth detection to work. +[] +let FitOrHide (items: ReactElement list) = + let elRef = React.useRef(None) + + let checkOverflow () = + match elRef.current with + | Some el when not (isNull el.parentElement) -> + let parent = el.parentElement + let count = childCount el + Seq.init count id |> Seq.iter (fun i -> (childAt el i)?style?display <- "") + let rec hideUntilFits i = + if hasOverflow parent && i < count then + (childAt el i)?style?display <- "none" + hideUntilFits (i + 1) + hideUntilFits 0 + | _ -> () + + React.useEffect(fun () -> checkOverflow ()) + + React.useEffect((fun () -> + match elRef.current with + | Some el when not (isNull el.parentElement) -> + let observer = createResizeObserver (fun _ -> checkOverflow ()) + observeElement observer el.parentElement + React.createDisposable (fun () -> disconnectObserver observer) + | _ -> + React.createDisposable ignore + ), [| |]) + + Html.span [ + prop.ref (fun el -> elRef.current <- if isNull el then None else Some el) + prop.className "fit-or-hide" + prop.children ( + items |> List.mapi (fun i item -> + Html.span [ prop.key i; prop.children [ item ] ]) + ) + ] + +let private commitGridElement (m: WorkMetrics) = + let displayCount = min m.CommitCount 90 + let overflow = m.CommitCount - displayCount + React.fragment [ + Html.span [ + prop.className "commit-grid" + prop.children (List.init displayCount (fun _ -> Html.span [ prop.className "commit-square" ])) + ] + if overflow > 0 then + Html.span [ prop.className "commit-overflow"; prop.text $"+{overflow}" ] + ] + +let private diffStatsElement added removed = + Html.span [ + prop.className "diff-stats" + prop.children [ + Html.span [ prop.className "diff-added"; prop.text $"+{added}" ] + Html.text " " + Html.span [ prop.className "diff-removed"; prop.text $"-{removed}" ] + ] + ] + +let workMetricsItems (metrics: WorkMetrics option) : ReactElement list = + match metrics with + | None -> [] + | Some m when m.CommitCount = 0 -> [] + | Some m -> + [ + commitGridElement m + if m.LinesAdded <> 0 || m.LinesRemoved <> 0 then + diffStatsElement m.LinesAdded m.LinesRemoved + ] + +let workMetricsView (metrics: WorkMetrics option) = + match metrics with + | None -> Html.none + | Some m when m.CommitCount = 0 -> Html.none + | Some m -> + Html.span [ + prop.className "work-metrics" + prop.children [ + commitGridElement m + match m.LinesAdded, m.LinesRemoved with + | 0, 0 -> Html.none + | added, removed -> diffStatsElement added removed + ] + ] diff --git a/src/Client/index.html b/src/Client/index.html index 2dedb31..2d35a4f 100644 --- a/src/Client/index.html +++ b/src/Client/index.html @@ -71,6 +71,10 @@ .beads-inline { display: flex; gap: 4px; align-items: center; color: #a6adc8; } .card-header { display: flex; align-items: center; gap: 8px; margin-bottom: 12px; } + .header-info { display: flex; flex: 1 1 auto; min-width: 0; overflow: hidden; align-items: center; gap: 8px; } + .header-info .branch-name { flex: 1 1 auto; min-width: 100px; } + .fit-or-hide { flex-shrink: 0; display: flex; align-items: center; gap: 4px; } + .fit-or-hide > span { flex-shrink: 0; display: flex; align-items: center; gap: 2px; } .branch-name { font-weight: 600; font-size: 0.95em; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; color: #cdd6f4; } .terminal-btn, .new-tab-btn, .editor-btn, .delete-btn, .resume-btn { diff --git a/src/Tests/DashboardTests.fs b/src/Tests/DashboardTests.fs index a3b3608..ecc17ad 100644 --- a/src/Tests/DashboardTests.fs +++ b/src/Tests/DashboardTests.fs @@ -1135,7 +1135,7 @@ type DashboardTests() = member this.``Commit overflow indicator shown for high commit count``() = task { let overflows = this.Page.Locator(".wt-card .commit-overflow") - do! overflows.First.WaitForAsync(LocatorWaitForOptions(Timeout = 5000.0f)) + do! overflows.First.WaitForAsync(LocatorWaitForOptions(Timeout = 5000.0f, State = WaitForSelectorState.Attached)) let! count = overflows.CountAsync() Assert.That(count, Is.GreaterThanOrEqualTo(1), "Fixture has branch with 95 commits (>90); overflow indicator should be present") @@ -1220,8 +1220,8 @@ type DashboardTests() = [] member this.``Work metrics appear in card header``() = task { - let metricsInHeader = this.Page.Locator(".wt-card .card-header .work-metrics") - do! Assertions.Expect(metricsInHeader.First).ToBeVisibleAsync(LocatorAssertionsToBeVisibleOptions(Timeout = 5000.0f)) + let metricsInHeader = this.Page.Locator(".wt-card .card-header .commit-grid") + do! metricsInHeader.First.WaitForAsync(LocatorWaitForOptions(Timeout = 5000.0f, State = WaitForSelectorState.Attached)) let! count = metricsInHeader.CountAsync() Assert.That(count, Is.GreaterThanOrEqualTo(1), "Work metrics should be inside card header") } @@ -1231,7 +1231,8 @@ type DashboardTests() = task { do! (compactBtn this.Page).ClickAsync() - let metricsInHeader = this.Page.Locator(".wt-card.compact .card-header .work-metrics") + let metricsInHeader = this.Page.Locator(".wt-card.compact .card-header .commit-grid") + do! metricsInHeader.First.WaitForAsync(LocatorWaitForOptions(Timeout = 5000.0f, State = WaitForSelectorState.Attached)) let! count = metricsInHeader.CountAsync() Assert.That(count, Is.GreaterThanOrEqualTo(1), "Work metrics should be inside compact card header") }