refactor(ui): decouple tui from slack I/O and core by laraibg786 · Pull Request #212 · gammons/slk

laraibg786 · 2026-09-13T19:09:41Z

internal/ui no longer does I/O itself. Every call it makes to Slack, SQLite, the filesystem, the clipboard, the external editor, or the OS goes through an interface in the new internal/core package, and cmd/slk wires the implementations. A test enforces the boundary.

Nothing changes for the user. There are no bug fixes and no new features. The diff is code motion, type aliases and rewiring.

Why

Diagnosis: when something breaks between the TUI and the app, the suspects are the methods on one interface, not closures spread across app.go and main.go.
Testing: the App can be tested with fakes for exactly the methods a test cares about, with no real filesystem, clipboard or process.
Fewer conflicts: new I/O has one obvious place to go, a port in core/services.go. boundary_test.go fails any change that reaches past it.

What moved

Port (`internal/core`)	Replaces	Implemented by
`ChannelService`, `MessageService`, `ThreadService`, `ReactionService`, `SearchService`	the same five interfaces, moved from `internal/ui/services.go`	`main.go` closures (unchanged bodies)
`FileService`	`SetUploader`, `SetFileDownloader`	upload closure, `filedl`
`DesktopService`	`SetClipboardReader`, `SetStatusReporter`, the `launchOS`/`os.Stat` defaults, the thread-export writer	`launchOS` (moved to `cmd/slk`), clipboard readers, `os.Stat`, `export.SaveThread`
`EditorService`	Ctrl+E's temp file and `exec.Cmd`	new `internal/editor` (verbatim move)
`PresenceService`	`SetStatusSetter`, `SetTypingSender`	`main.go` closures
`SettingsService`	`SetThemeSaver`, `SetWidthSaver`	`main.go` closures
`UnreadService`	`SetReadStateReader`, `SetWorkspaceUnreadReader`	`main.go` closures
`WorkspaceService`	`SetWorkspaceSwitcher`	`main.go` closure
`AvatarService`	`SetAvatarFunc`	avatar cache
`ImageFetcher`	the concrete `*image.Fetcher` field	`*image.Fetcher`

The value types shared by the TUI and the app (MessageItem, ReadState, Theme, PendingAttachment, Block Kit types, and so on) now live in core. Their old names stay as type aliases, so call sites don't change.

Commits

Each commit builds and passes the full suite on its own. They are best reviewed in order, with git show --color-moved.

Commit	What it does	Size
`test(ui): pin the TUI's collaborator seams`	Tests only, passing on current `main`: 36 + 2 scenarios that drive the App the way a user would and pin what crosses each seam	+1069
`refactor(core): move the data the TUI and engine share`	types into `core`, aliases left behind	±400
`refactor(core): move the five service ports`	`ui/services.go` → `core/services.go`, `tea.Msg` → `core.Msg`	±400
`refactor(ui): put the TUI's remaining collaborators behind core ports`	the new ports and setters, `main.go` rewiring	+708/−326
`refactor(styles): read custom theme files through fs.FS`	`LoadCustomThemes(fsys fs.FS)`	±10
`refactor(ui): run the external editor through a core port`	Ctrl+E I/O → `internal/editor`	+197/−69
`test(ui): guard the TUI/core boundary`	`boundary_test.go`	+142
`test(ui): drop an unused typing-sender helper`	lint	−8
`docs: point AGENTS.md at internal/core`	invariant and helper table	±9

How regressions are ruled out

Frozen seam tests. seams_test.go and styles/seams_test.go land first and pass on main, then stay byte-identical through every later commit. The wiring they go through lives in seams_wiring_test.go, the only test file the refactor commits rewrite.
Boundary test. On current main it reports 28 violations; on this branch it reports none.
Goldens. All 8 frames are byte-identical; testdata/golden isn't in the diff.
Other existing tests. Changes are mechanical only: renamed setters, the export and editor tests moving with their functions, and a wiring line where a test used to rely on the real OS default.

Not changed on purpose

Environment lookups (TMUX, os.UserHomeDir, $VISUAL/$EDITOR) stay in the TUI. It reads its own terminal; the boundary test allows them.
The engine still constructs ui message types, and the implementations are still closures inside run(). Extracting them is Phase 2 work.
launchOS and the clipboard readers moved verbatim and are still untested, as before.

Manual test plan

These are the paths that changed wiring. Each should behave the same as on main:

Characterizes every place the TUI hands work to the rest of the app: presence, typing, theme and width saves, uploads, workspace switching, unread readers and the status reporter, avatars, clipboard paste (text, image, file path), thread export, image preview fetch, and the channel/message/thread service calls whose results the reducers rewrite. Plus the custom theme loader. These are the regression net for moving the collaborators behind internal/core ports. Scenarios live in seams_test.go; how each collaborator is installed lives in seams_wiring_test.go, which is the only file the refactor may change.

…core The ports the TUI will depend on need to name these types, and a port package can't import the TUI. Move the definitions verbatim and leave aliases behind, so neither side changes how it spells them: - messages: MessageItem, Attachment, ThumbSpec, ReactionItem - blockkit's Block Kit data (now internal/core/blocks) - channelfinder.Item, reactionpicker.EmojiEntry, compose.PendingAttachment - presencemenu.Action, themeswitcher.ThemeScope - cache.ReadState, cache.ThreadSummary, config.Theme With the last three in core the TUI no longer imports internal/cache or internal/config at all. blockkit's sealed blockType() marker moved with the types, so render.go and one test reach it through blocks.TypeName instead.

ChannelService, MessageService, ThreadService, ReactionService and SearchService, with their closure-bundle constructors, now live in core, so the TUI depends on ports it doesn't own and cmd/slk builds them without going through the TUI package. core can't name tea.Msg (a defined type in ultraviolet), so the ports return core.Msg (= any) and core.Cmd (= func() core.Msg). Closure bodies in main.go are unchanged; only their result types moved. The three call sites that need a tea.Cmd go through teaCmd, which keeps nil nil so the existing `c != nil` guards still hold. The per-method test helpers used to read the installed adapter's closures back by type assertion; the adapter is unexported in core now, so they remember what each test App was wired with instead.

The App still took a dozen bare callbacks, a concrete *filedl.Downloader and *image.Fetcher, and made its own OS calls (exec for links and files, the native and wl-paste clipboards, os.Stat for paste-a-path, the thread export's MkdirAll/WriteFile). They now go through services in core: FileService upload, download DesktopService open, clipboard, stat, thread export, status command PresenceService set status, typing SettingsService theme, sidebar width UnreadService sidebar and workspace-rail read state WorkspaceService switch AvatarService rendered avatars ImageFetcher what *image.Fetcher already provides cmd/slk wires them from the same closures as before; only the calls that install them changed. launchOS and the clipboard readers moved to cmd/slk, the export write to internal/export.SaveThread. Services default to nil where the old callbacks were nil-checked, so "not wired" behaves as before. DesktopService defaults to a no-op instead, because its callbacks used to fall back to the real OS, and an unwired Stat reports files as missing rather than returning (nil, nil). The two paste-a-path tests that leaned on that real fallback now wire os.Stat explicitly.

LoadCustomThemes did its own os.ReadDir/os.ReadFile. It now takes an fs.FS and main passes os.DirFS(themesDir), so the TUI no longer touches the filesystem itself. Same listing order, same skips, same silent return when the directory is missing.

Ctrl+E's temp file and editor process move verbatim to internal/editor, which cmd/slk wires as the new core.EditorService. The TUI keeps the policy: which compose box, the lock, the toasts, and trusting the file over a non-zero exit, now recognised by the error's ExitCode method rather than its *exec.ExitError type.

Fails when a non-test file under internal/ui imports infrastructure (the Slack client, slackhttp, cache, config, filedl, export, net/http, os/exec, the native clipboard, ...) or calls the filesystem, or names the concrete image fetcher; and when internal/core imports the TUI or I/O packages. slack-go stays allowed in blockkit, whose input it is. Run against the tree before this series it reports the 22 places the TUI used to do its own I/O.

Nothing wires typing that way; golangci-lint flagged it.

The UI invariant named internal/ui/services.go and five service interfaces; the ports now live in internal/core and the boundary has a test. Also list the per-method service helpers tests use.

laraibg786 · 2026-09-13T19:24:37Z

I'll mark this ready for review once I've finished manual testing (the checklist in the description).

This is a first step rather than a full cleanup. It moves the TUI's I/O behind interfaces in internal/core, so the rest of the app can be tested without the UI, and a problem between the two can be traced to one interface instead of closures scattered across app.go and main.go. Much of what's in the architecture plan is still ahead: splitting run(), App.Wire(Deps), and so on. I've deliberately left those out to keep the blast radius small and the review manageable. The "Not changed on purpose" section lists what I skipped.

Suggestions welcome. I know this will conflict with some open PRs; #109 is the one that needs real changes, since its new service belongs in core/services.go now. I'm happy to rebase this after whatever you want to land first, or to help rebase the others onto it.

laraibg786 · 2026-09-14T13:14:39Z

@gammons the PR is ready from my side, i have tested the scenarios listed in the description manually, the test suite is green. please review and let me know if you want me to explain something. Your review and suggestions are highly welcome.

gammons · 2026-09-14T23:43:16Z

Thanks for this — it's a well-built PR, and the verification story is the best part. Before getting into specifics I should give you some context you don't have.

Context: there's a refactor plan in flight

I've been working with an agent over the last week on the parts of the architecture that hurt most. The output is on main:

docs/superpowers/plans/2026-09-06-architecture-refactor.md — measured baseline at 4184e60, eight findings, a six-phase sequence
docs/superpowers/specs/2026-09-06-phase0-test-safety-net-design.md + its plan — Phase 0, which is what landed the goldens, the newTestApp harness, the mode-handler characterization and the flake fixes you're building on top of

Phase 0 is done. Phases 1–6 hadn't started, which is why this PR doesn't collide with anything. Worth reading the tracking doc if you're planning more of this — not to constrain you, but so we're not solving the same thing twice from different directions. That's the failure mode I'm most worried about right now.

Your "Phase 2" vs the plan's Phase 2 — partial overlap

You wrote: "the implementations are still closures inside run(). Extracting them is Phase 2 work."

That maps almost exactly onto the plan's Phase 2, step 6: extract wireCallbacks (541 lines) to cmd/slk/callbacks.go with an explicit deps struct. Same work, same motivation.

Two things you'd hit that the plan already scoped, though:

A prerequisite. p *tea.Program is declared at main.go:1186 and not assigned until :1972, so every closure in run() depends on the nil-then-set pattern. You can't lift wireCallbacks out until p becomes a send func(tea.Msg) indirection — that's step 4, and it exists specifically to unblock step 6. newUserResolver, membership.New and resolveDMNames already take the callback form, so the pattern is established.

Three live data races in the same code. Phase 2 steps 1–3 are not cleanup, they're bug fixes:

activeTeamID (main.go:1188) is a plain string written from the UI goroutine (:1906) and from N connect goroutines (:2035, :2042), and read from every WebSocket goroutine via the isActive closure (:2062). It's also redundant with router.Active().TeamID.
workspaces (main.go:1187) is a plain map written from N connect goroutines at :2022 — and it duplicates router.all, written on the very next line.
cfg is mutated in place by SetThemeSaver/SetWidthSaver on the UI goroutine while a copy sharing the cfg.Workspaces map lives in every rtmEventHandler (:2070).

The doc comment at main.go:276-278 claims all router.all writes precede p.Run. They don't — connect goroutines start at :2001, p.Run() is at :2186. If you take this on, please fix the races and the comment in the same pass, and ideally before the extraction, so the extraction diff stays reviewable.

And you've found a gap in the plan. Your other deferral — "the engine still constructs ui message types" — isn't in any of the six phases, and after this PR it's the most visible asymmetry left: I/O now flows ui → core ← cmd, but messages still flow cmd → ui directly. cmd/slk imports internal/ui in four non-test files and constructs 73 ui.*Msg values. I'll add that to the tracking doc as its own item rather than smuggling it into Phase 2, since it's a different shape of problem.

One note on the net effect here: main.go went 5,256 → 5,270 on this branch. That's fine for what the PR set out to do, but it means the largest file in the repo got marginally larger while the I/O left the TUI. Phases 1–2 are where that gets paid back, and this PR makes them more urgent rather than less.

You corrected something I got wrong

The invariant I had in AGENTS.md said internal/ui must not import networking and that all I/O already crossed the five service interfaces. Your boundary test finds 28 violations on main — app.go alone imports os/exec, cache, config, export, filedl and clipboard, and calls os.MkdirAll, os.WriteFile and os.Stat directly; editor.go does five raw os calls; styles/themes.go reads the filesystem.

The original claim was narrowly true about networking imports in the root package, and I over-generalised it to "does no I/O." Your rewrite is right, and backing it with an enforcing test instead of prose is exactly the convention the file asks for. Thanks for maintaining the helper table too.

What I verified

I didn't take the claims on trust — the regression argument is the whole review here, so:

38 seam tests pass on unmodified main. Confirmed (I initially thought styles/seams_test.go didn't compile there; I'd missed the 7-line styles/seams_wiring_test.go shim — my error, the design is sound).
They stay byte-identical across all 9 commits. Blob a7e6abec at every one.
Boundary test: exactly 28 violations on main, 0 here.
Goldens untouched — zero files under testdata/golden in the diff.
Builds clean, go test ./... -race green across 56 packages, golangci-lint 0 issues.

The frozen-seam-test technique — land the scenarios first so they pass on main, freeze them, and let only a separate wiring file change — is a stronger regression argument than what the plan prescribes for the later phases. I'd like to adopt it for Phases 1–4.

Three things I'd like addressed

1. TestCoreDependsOnNeitherTUINorIO promises more than it enforces. Its ban list is internal/ui/*, charm.land/*, os, os/exec, net, net/http, database/sql. It omits internal/image, which core imports directly and which itself imports net/http, os and os/exec. core.ImageFetcher's signatures are built from imgpkg.FetchRequest, FetchResult, Protocol, Render and KittyRenderer (core/services.go:894-898), so the port layer's vocabulary is tied to an I/O package.

I don't think this blocks the PR — parallel core-owned types for the whole image pipeline is a real cost and probably not worth paying today. But a test whose name over-promises is how a boundary quietly erodes. Either add internal/image to the ban list and introduce core types, or narrow the test name and add a comment saying the fetcher is a deliberate exemption and why.

2. Is core/services.go at 981 lines already becoming a grab-bag? It holds 14 interfaces and 38 struct/func types. The interfaces are ports; the *Funcs bundles are adapters. Splitting them — ports in core, bundles next to their implementations, or at least separate files — would keep core a boundary rather than the next file we have to break up. Not a blocker, but it's the pattern this whole effort is unwinding, so I'd rather ask now.

3. The five type aliases (channelfinder.Item, compose.PendingAttachment, presencemenu.Action, reactionpicker.EmojiEntry, themeswitcher.ThemeScope) are the right call for keeping this diff mechanical. Could you open a follow-up issue to remove them? Otherwise they become permanent double-naming, and nobody will remember they were meant to be temporary.

Also

Two production bugs surfaced during Phase 0 that are adjacent to code you touched, so you may bump into them:

View() renders 6 columns wider than the terminal width, wrapping the status row #181 — View() returns a frame six columns wider than the terminal, which wraps the status row. golden_test.go encodes it as goldenStatusOverflow = 6; when it's fixed that constant goes to 0 and the goldens get re-blessed.
Thread header separator uses ASCII '-' where every other separator uses '─' #182 — the thread header separator uses ASCII - where every other separator uses ─.

Neither is yours to fix here.

Approving with the three comments above — none of them blocks merging, and I'd rather land this and iterate than hold it. Nice work on the verification discipline; it's the part I'd most like to see repeated.

Interfaces (ports) and their Funcs/adapter/constructor plumbing were in one 981-line file. Pure move, no behavior change: ports.go holds the 14 service interfaces and ClipboardFormat; adapters.go holds the Funcs bundles, adapter structs, constructors, and closure typedefs.

It named itself for a guarantee it didn't enforce: core imports internal/image, which itself reaches net/http and os. Rename to TestCoreDoesNotImportTUIOrDirectIO and document the image port as a deliberate exemption instead of silently allowing it under a misleading name.

laraibg786 · 2026-09-15T10:22:21Z

Thanks for your review. I have fixed your concerns and implemented your suggestions.

I have updated the test name to accommodate the reduced scope of the test since we are keeping the internal/image since it would be too much refactor for this PR.
core/services has been reduced to two ports and adopters files to make sure that this does not end up being the dump bag anyways.
I have opened the issue Remove the five temporary type aliases from the core move #217 to document the issue so that this can later be fixed.

This should be good for merge now.

gammons · 2026-09-16T10:00:01Z

Looks great. thank you @laraibg786 !

laraibg786 added 9 commits September 13, 2026 22:47

test(ui): drop an unused typing-sender helper

e8bf2a4

Nothing wires typing that way; golangci-lint flagged it.

docs: point AGENTS.md at internal/core

7fda0f4

The UI invariant named internal/ui/services.go and five service interfaces; the ports now live in internal/core and the boundary has a test. Also list the per-method service helpers tests use.

laraibg786 marked this pull request as ready for review September 14, 2026 13:13

laraibg786 added 2 commits September 15, 2026 08:20

laraibg786 mentioned this pull request Sep 15, 2026

Remove the five temporary type aliases from the core move #217

Open

This was referenced Sep 15, 2026

feat: show other people's status, DND and huddles #219

Merged

Show other people's custom status, DND and huddle state #211

Closed

gammons merged commit 549285e into gammons:main Sep 16, 2026
3 checks passed

laraibg786 deleted the refactor/tui-ports branch September 16, 2026 10:33

laraibg786 mentioned this pull request Sep 16, 2026

fix(slk): close the activeTeamID and cfg.Workspaces connect races #221

Open

Navigation Menu

refactor(ui): decouple tui from slack I/O and core - #212

refactor(ui): decouple tui from slack I/O and core#212
gammons merged 11 commits into
gammons:mainfrom
laraibg786:refactor/tui-ports

laraibg786 commented Sep 13, 2026 •

edited

Loading

Uh oh!

laraibg786 commented Sep 13, 2026

Uh oh!

laraibg786 commented Sep 14, 2026

Uh oh!

gammons commented Sep 14, 2026

Uh oh!

laraibg786 commented Sep 15, 2026

Uh oh!

gammons commented Sep 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

laraibg786 commented Sep 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What moved

Commits

How regressions are ruled out

Not changed on purpose

Manual test plan

Uh oh!

laraibg786 commented Sep 13, 2026

Uh oh!

laraibg786 commented Sep 14, 2026

Uh oh!

gammons commented Sep 14, 2026

Context: there's a refactor plan in flight

Your "Phase 2" vs the plan's Phase 2 — partial overlap

You corrected something I got wrong

What I verified

Three things I'd like addressed

Also

Uh oh!

laraibg786 commented Sep 15, 2026

Uh oh!

gammons commented Sep 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

laraibg786 commented Sep 13, 2026 •

edited

Loading