feat: migrate to Effect 4 and Solid 2 RC - #48
Open
joncrangle wants to merge 11 commits into
Open
joncrangle wants to merge 11 commits into
joncrangle wants to merge 11 commits into
Conversation
joncrangle
force-pushed
the
codex/effect-solid2-migration
branch
from
September 18, 2026 23:20
13346d2 to
1a71568
Compare
Remove unused DriveItem component and importer-less components barrel. Trim AppView union to the 7 live views and collapse the footer switch. Remove production-unused isNavigationKey and its tests. Fix dead Header padding ternary. Trim AGENTS.md to evergreen guidance. Add just fmt-check and enforce it in CI.
Owner
Author
Merge-gate landing checklistWhen Solid 2 / Effect 4 go stable and OpenTUI ships a Solid 2-native renderer, this is exactly what needs to change (verified 2026-09-20 — all three are still RC upstream, so nothing to do yet):
|
Seven exports were declared but referenced nowhere in the repo: - PaneId (superseded by FocusedPane in types/keyboard.ts) - SyncOptions, SyncEpisode, SyncResult - DriveEpisode, DrivePodcast (a self-contained dead pair; DriveScan declares its own inline shape for buildDriveIndex) - createPodcastServiceTest (siblings createSyncEngineTest, createDriveScanTest, createMetadataEditorTest and createFileSystemTest are all still in use)
Three independent bugs in the sync path.
Sync success messages were silently discarded. startSync returns a
message on both success paths ("All episodes already synced" and "Sync
complete"), but the caller only read it on the failure branch, so the
message was computed and thrown away: sync forty files, the popup
closes, and nothing confirms it happened.
Rather than push a non-error through the error channel, add a dedicated
successMsg field and setSuccessMsg action, rendered on its own line in
Colors.text.success. Cleared explicitly at the same points errorMsg is
(new sync, refresh, drive selection, cancel), matching the existing
no-auto-expiry idiom.
The transfer file counter was off by one. The engine emitted a 0-based
currentIndex from Stream.zipWithIndex while the popup renders it as a
done/total counter, so it read 0/3 while the first file copied. Three
signs this is a bug and not a choice: the engine's own log line already
used `i + 1`, the test double already emitted `i + 1`, and the counter
is display-only. Fixed the producer at all three emit sites so
production matches the already-tested contract, rather than adjusting
the consumer. Includes the "Tagging:" emit, which carried the same
off-by-one.
The ID3 artist tag was always blank. ZMTPODCAST.ZAUTHOR was populated
all along, but the query never asked for it: db.worker.ts selected only
p.ZTITLE, and groupEpisodesByPodcast hard-coded `author: ""`, which
reached the tag writer as `artist: ""`. Select p.ZAUTHOR, add author to
EpisodeRow and to the required Episode interface, carry it through the
row mapping, and read it off the grouped episodes with a fallback so a
blank is never written.
Also discard a copy whose tag write failed. The old code swallowed the
error and left the file on the drive with wrong or missing tags. It is
now logged and unlinked, so the next run re-copies and re-tags. This
preserves the existing rule that a failed tag never fails the sync.
Follows the previous sync fixes, addressing two gaps they left.
Make successMsg and errorMsg mutually exclusive in the store actions.
The two could previously both be set, so a red error and a green
"Sync complete" could render at once: after a successful sync, a
hotplug-triggered rescan could fail and set an error without clearing
the stale success. There are eight setErrorMsg call sites and any of
them could fire in that window, so the invariant now lives in the
setters rather than being remembered per call site. Three explicit
setSuccessMsg("") calls that paired with setErrorMsg("") are gone as
redundant.
Surface files discarded because tagging failed. Those are logged and
unlinked so the next run re-copies, but the sync still reported a
green "Sync complete" with nothing on the drive, which is its own kind
of lie. SyncProgress gains a required `discarded` counter, and
startSync folds the final count into the message: "Sync complete
(2 files discarded: tagging failed)". The terminal progress event uses
Stream.suspend rather than Stream.succeed, because the latter builds
its value when the stream is assembled, so it captured the count as 0
before any file was tagged.
Also log an unlink that fails. createPlan skips any destination that
exists, so a leftover half-tagged file is never re-copied; the failure
previously left no trace at all.
Finally, do not overwrite a failed rescan's error with a success
message: loadDrivePodcastsEffect now reports whether it succeeded, and
the success is only set when it did.
Convert the remaining `as SyncProgress` and `as SyncPlan` casts to
checked types, since an `as` assertion permits a missing property and
the new field would have compiled while emitting undefined.
useAppLogic had no test file, so the string the whole discarded-files change exists to produce was unverified, as was the mutual exclusion of successMsg and errorMsg along the paths the hook actually takes. Extract syncSuccessMessage so the singular/plural branch can be pinned directly, and cover the hook against the real store: a sync refused for a missing drive sets an error and no success, a stale success does not survive a new sync or a drive selection, and a bare rescan leaves the status alone. Note that loadDrivePodcasts is a rescan, not a selection: only selectDrive clears the status, so the two are asserted separately.
Remove src/theme/index.ts and src/types/index.ts. Neither is imported: every consumer reaches the specific module (@/theme/colors, @/types/drive) rather than the barrel, so both re-exports only ever shadow the real modules with a second import path. Also clear rust/, 6.2G of cargo build output left in the working tree from the gpui port. It held only dist/ and target/ with no Cargo.toml, so it was purely regenerable artifacts, and it was not gitignored, which meant a git add -A would have tried to stage 6.2G.
Error rendering used `err instanceof Error ? err.message : String(err)` in
six places. That is wrong for Effect: a Data.TaggedError subclass such as
DriveScanError IS an instance of Error, but its `message` is empty, so the
ternary produced "" — setErrorMsg("") renders nothing at all, and a failed
drive scan became a completely invisible error. A new test for the
post-sync rescan gate failed against exactly that, with state.errorMsg
empty after a scan that had demonstrably failed.
describeError prefers a real message, then a string or Error cause, then
the tag, and never returns something meaningless: an empty string, a bare
class name, "[object Object]" and "null" all become "Unknown error".
It unwraps an Effect Cause, so the sync-failure path reads "disk full"
instead of "Cause([Fail(SyncError (cause: Error: disk full))])", and names
an interruption "Cancelled" rather than dumping the wrapper.
useAppLogic now takes an optional service layer, defaulting to the live
one, so tests can drive the real control flow. Both run and runFork close
over that parameter; runFork previously reached for the module-level
AppLayer, which would have silently ignored an injected layer. This is the
only way to reach the failure paths, since they need an unplugged cable or
a broken database — the live scanDrive returns [] for a missing Podcasts
folder and swallows its own list errors, so it never fails on a tempdir.
Covered: the rescan gate in both directions, the discarded-files message,
the exclusive status channel, and describeError including a hostile
constructor name, since it runs inside catch handlers and the Errored
boundary where a throw would crash the app.
The existing syncEngineLive test asserts the artist reaching MetadataEditor, which pins the ZAUTHOR chain only up to that boundary. Nothing checked the file itself, so the one link that produced the original bug — the artist frame node-id3 writes — was untested. Writes a real file, tags it, then reads it back with node-id3 and asserts on the tag that actually landed. Reverting groupEpisodesByPodcast to the old `author: ""` makes this fail with an empty artist on disk, which is what every file podapple had synced before the fix. Also covers a missing and a whitespace-only ZAUTHOR, since the fallback has to guarantee a blank is never written.
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
1 participant
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.
Summary
SystemError: Bad file descriptor.Validation
bun install --frozen-lockfilejust checkjust test— 149 passingjust lintjust buildMerge gates
This PR is intentionally a migration branch and should remain unmerged until:
The OpenTUI packages are pinned to 0.5.11 while the patch is present to prevent version drift.