Refactor App.fs into focused view and state-slice modules#82
Merged
Conversation
…cheduler footer) Relocate status-overview + scheduler-footer pure views into new src/Client/OverviewViews.fs (compiles before AppTypes, after CanvasState.fs). Move shared formatters stepStatusClassName/stepStatusText/relativeEventTime into Components.fs per spec Decision 7; App.fs keeps thin re-export aliases so card-side call sites stay untouched. Repoint App.fs view to OverviewViews.schedulerFooter. Pure relocation, no logic change.
…-place card refactor) Introduce Msg-free CardViewProps (8-field model read-slice) and CardCallbacks records mirroring CanvasPaneCallbacks. Convert card leaf helpers and composite views (compactWorktreeCard/worktreeCard/renderCard/repoSectionHeader/repoSection) from raw dispatch to the records; build cardProps/cardCallbacks in view() and update the repoSection call site. Drop pre-existing dead args (renderCard repoId, worktreeCard canvasPaneOpen) and the archiveSection dispatch wrapper. Update spec Decisions with the leaf-helper-conversion note.
…CardViews.fs Created src/Client/CardViews.fs (module CardViews) holding the CardViewProps/ CardCallbacks records plus all card leaf helpers moved verbatim from App.fs: icons, action buttons, beads/build badges, sync helpers, event-log and PR helpers, class/text/format helpers, and canvasEventEntry/canvasEventLog. CardViews.fs compiles before AppTypes.fs. App.fs adds 'open CardViews' and drops the now-dead stepStatusClassName/stepStatusText aliases. Composite card views intentionally remain in App.fs for Step 4.
…fs and wire view() Move 10 composite card defs (canResumeSession, compactWorktreeCard, worktreeCard, renderCard, skeletonCard/skeletonGrid, sortLabel, providerIcon, repoSectionHeader, repoSection) verbatim from App.fs into CardViews.fs; drop 6 now-dead Components aliases. view() call sites resolve via existing 'open CardViews'.
…el.Mascot) Introduce MascotState sub-state (EyeDirection/LastActivityTime/ActivityLevel + empty, randomEyeDirection, computeActivityLevel, idle thresholds) compiled before AppTypes. Embed as Model.Mascot (drop the 3 flat fields) and repoint init/DataLoaded/Tick/UserActivity/appSubscriptions/header to model.Mascot.*. Update 4 test Model literals/reads (CanvasAwareness, ConfirmModal, CreateWorktree, IdleDetection).
…ot slice) MascotView.fs (before AppTypes): viewEyeOpen reshaped to take a MascotState slice; viewEyeRolledBack/viewEyeClosed verbatim. Header wired to MascotView.*. MascotUpdate.fs (after CanvasUpdate.fs): tickActivity + userActivity arm bodies and the activityDetection subscription. Root update delegates (flat Msg kept; Tick stays in root for canvas-event expiry + poll). appSubscriptions points to MascotUpdate.activityDetection. Behavior-preserving extraction.
…portunity B) Move focusedWorktreeCanvasDoc + the canvas-pane wiring block out of App.fs's view into a new CanvasView.fs module exposing view(model, dispatch). Registered in Client.fsproj after MascotUpdate.fs, before App.fs. App.fs 847->794 lines; pure mechanical relocation, no behavior change.
…ction specs - review/rules/immutability.md: never recommend or use a ref cell to dodge the rule; let mutable is the sanctioned mechanism when mutation is justified, and must carry an inline comment explaining why an immutable solution doesn't fit. - MascotUpdate.fs: document the throttle-closure let mutable (per user-idle-detection). - Delete docs/spec/app-fs-view-extraction.md and its future/ companion (red-flag one-off -extraction filenames; feature complete). Strip dangling spec refs from CanvasView/MascotView/MascotUpdate headers and fix the dead link in canvas-pane.md.
Move idle detection and activity updates into ActivityState and ActivityUpdate. Keep mascot state to gaze only and pass observed ActivityLevel into the eye view. Repoint tests and remove MascotUpdate.
After the App.fs view extraction + activity/mascot SoC split, several specs still pointed at old App.fs locations. Update their Key Files tables / module references (canvas-pane, user-idle-detection, worktree-monitor, resume-last-session, contextual-actions) to the new modules (ActivityState/ActivityUpdate, CardViews, OverviewViews, MascotState/MascotView, CanvasView). Behavior text unchanged. Add docs/spec/future/code-improvements.md: a running backlog of improvement candidates plus the worktree -> /bd-plan -> /bd-execute -> PR workflow.
There was a problem hiding this comment.
Pull request overview
This is a behavior-preserving code-quality refactor that breaks the monolithic src/Client/App.fs into focused view and state-slice modules, and—separately—untangles user-activity/idle-detection state from the mascot. The Elmish architecture is unchanged: a flat Msg and a single update are preserved, and extracted functions are pure body/view relocations (mirroring the existing CanvasState/CanvasUpdate seam). The card views move to records (CardViewProps/CardCallbacks) instead of long positional argument lists, narrowing each leaf's capability from raw dispatch to named actions. I verified the new module compile order, that every callback/prop maps to the previous dispatch behavior, and that all references (tests included) were updated with no dangling symbols.
Changes:
- Extract
App.fsview code intoOverviewViews.fs,CardViews.fs,MascotView.fs,MascotState.fs, andCanvasView.fs; relocate shared formatters intoComponents.fs. - Split activity/idle state out of the mascot into
ActivityState.fs+ActivityUpdate.fs;Modelnow carriesActivityandMascotslices (replacingEyeDirection/LastActivityTime/ActivityLevel). - Documentation/review-rule housekeeping: refresh
Key Filesreferences, tightenimmutability.md(forbidref-cell dodges), delete the stale extraction spec, and add acode-improvements.mdbacklog.
Show a summary per file
| File | Description |
|---|---|
src/Client/App.fs |
Slimmed to init/update/subscriptions/top-level view; wires CardViewProps/CardCallbacks and delegates to new modules |
src/Client/AppTypes.fs |
Model gains Activity/Mascot slices, drops EyeDirection/LastActivityTime/ActivityLevel |
src/Client/CardViews.fs |
New module: all worktree-card rendering via props/callback records (replaces 9 positional args) |
src/Client/OverviewViews.fs |
New module: status-overview row + scheduler footer (pure render) |
src/Client/CanvasView.fs |
New module: canvas-pane view wiring lifted from view |
src/Client/MascotView.fs / MascotState.fs |
New modules: eye SVGs + gaze slice (randomEyeDirection) |
src/Client/ActivityState.fs / ActivityUpdate.fs |
New modules: activity slice + thresholds/computeActivityLevel, and Tick/UserActivity/activityDetection bodies |
src/Client/Components.fs |
Relocated stepStatusClassName/stepStatusText/relativeEventTime as single source of truth |
src/Client/Client.fsproj |
Adds the seven new modules in valid compile order |
src/Tests/{IdleDetection,CreateWorktree,ConfirmModal,CanvasAwareness}Tests.fs |
Update to new Model shape and ActivityState-qualified helpers |
review/rules/immutability.md |
Clarifies justified, scoped let mutable is compliant; forbids ref-cell workarounds |
docs/spec/{worktree-monitor,user-idle-detection,resume-last-session,contextual-actions,canvas-pane}.md |
Refresh Key Files/module references to new layout |
docs/spec/future/code-improvements.md |
New improvement backlog (line-count figure is off—see comment) |
docs/spec/future/app-fs-view-extraction.md |
Deleted (transient plan; no remaining references) |
Copilot's findings
- Files reviewed: 23/23 changed files
- Comments generated: 1
|
|
||
| ## Done | ||
|
|
||
| - **App.fs view extraction** — `src/Client/App.fs` 1685 → 705 lines. Extracted |
Owner
Author
There was a problem hiding this comment.
Good catch — confirmed and fixed. Root cause: the figures came from PowerShell's Measure-Object -Line, which silently undercounts blank lines (~10% here). The authoritative (Get-Content).Count is 1861 on main and 795 now — a 1066-line (57%) reduction. Corrected in the backlog doc (commit 5b94c21) and the PR description.
The reported figures came from PowerShell's Measure-Object -Line, which undercounts blank lines (~10% here). The authoritative count ((Get-Content).Count) is 1861 on main and 795 now — a 1066-line (57%) reduction. Caught in PR review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
src/Client/App.fshad grown to 1861 lines, mixing the Elmishupdate, subscriptions, and three large families of pure-render view code (worktree cards, the mascot eyes, the status overview/footer) plus the canvas-pane wiring. The mascot "slice" also secretly co-owned the app's user-activity/idle state — which actually drives polling cadence, server telemetry, and canvas auto-display, not the eyes.This is a behavior-preserving code-quality branch: no features added or changed. The E2E suite (Playwright, asserting on DOM/CSS classes) proves the rendered UI is identical before and after.
Changes
App.fs view extraction (1861 -> 795 lines) — an evidence-driven hybrid: a vertical slice only where a feature owns cohesive state + behaviour; props/callback records where it is render-over-shared-state; plain extraction where there is no owned state.
OverviewViews.fs— status-overview row + scheduler footer (pure render).CardViews.fs— all worktree-card rendering, now takingCardViewProps+CardCallbacksrecords instead ofrepoSection's former 9 positional args.MascotState.fs/MascotView.fs— the mascot eyes as a gaze slice + eye SVGs.CanvasView.fs— the canvas-pane view wiring lifted out of the top-levelview.Components.fs(single source of truth).Msg+ singleupdatepreserved throughout (noCmd.mapsub-component split), mirroring the existing canvas seam.Activity / mascot separation of concerns — split user-activity / idle-detection state out of the mascot into a dedicated
ActivityState.fs+ActivityUpdate.fsslice (ownsLastActivityTime/ActivityLevel,computeActivityLevel, theTick/UserActivitybodies, and the activity subscription). The mascot is now a pure widget that observesActivityLevel;MascotUpdate.fsfell empty and was deleted.Review-rule + docs housekeeping
review/rules/immutability.md: forbid using arefcell to dodge the rule —let mutableis the sanctioned, comment-justified mechanism when mutation is genuinely required.app-fs-view-extractionspecs (red-flag-extractionfilename; transient implementation plan).Key Filesreferences incanvas-pane.md,user-idle-detection.md,worktree-monitor.md,resume-last-session.md, andcontextual-actions.mdto the new module layout (behaviour text unchanged).docs/spec/future/code-improvements.md— a running improvement backlog + the worktree ->/bd-plan->/bd-execute-> PR workflow.Tests
Behaviour-preserving; verified green at every step and end-to-end:
npm run build(Fable + vite): clean.App.fs795 lines; all seven new modules wired intoClient.fsprojin valid compile order.Each extraction task additionally passed a focused multi-model code review before the final verification.