diff --git a/CHANGELOG.md b/CHANGELOG.md index a8ae5ba..1744104 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- TUI: warnings emitted by successful updates in an apply-updates batch are + now readable — they render as a trailing section (blank separator, one + line per distinct warning) inside the same "update results" overlay that + lists each update's ✓/✗ line, instead of being folded into an aggregate + the overlay never showed. Identical warnings repeated across the batch + (a merged-pak recompile re-emits the same profile-level asset-conflict + diagnostics for every update that triggers it) are deduped on exact text, + and the status line's one-row `(N warnings)` count matches the deduped + section (#259). - Deploy output on a compile-mode game (Icarus) no longer presents merged mods as individual deployments (#255). The header drops the misleading `using ` claim, each mod's `✓` line is labeled by how its content diff --git a/internal/tui/actions_provider.go b/internal/tui/actions_provider.go index f9f52a8..5071ecf 100644 --- a/internal/tui/actions_provider.go +++ b/internal/tui/actions_provider.go @@ -211,12 +211,15 @@ type ActionOutcome struct { // (mutations.go), the apply-updates batch's confirm-time body, populates // this today - one "✓ → " line per successful update, // one "✗ : " line per failed one, in the SAME order the - // batch was applied. Every other ActionProvider call leaves this nil, - // same as ImportedProfile's own "" zero value above - app.go's - // actionDoneMsg handler treats a nil/empty ResultLines as "nothing to - // show" and opens no overlay for it. This is a TUI-side struct, not part - // of the ActionProvider interface itself, so adding it required no - // interface/method change on either provider (coreProvider/ + // batch was applied, plus (#259) one trailing section - a blank + // separator, then one line per distinct success-emitted warning - when + // any successful update carried Warnings (see applyUpdatesSequentially's + // doc comment for why they must ride here). Every other ActionProvider + // call leaves this nil, same as ImportedProfile's own "" zero value + // above - app.go's actionDoneMsg handler treats a nil/empty ResultLines + // as "nothing to show" and opens no overlay for it. This is a TUI-side + // struct, not part of the ActionProvider interface itself, so adding it + // required no interface/method change on either provider (coreProvider/ // prototypeProvider): renderers besides the update batch's are free to // ignore it entirely. ResultLines []string @@ -450,6 +453,28 @@ func (p *prototypeProvider) UninstallMod(_ context.Context, item ModItem) (Actio return ActionOutcome{Message: fmt.Sprintf("Uninstalled %q", item.Name)}, nil } +// prototypeMergeWarnings returns the canned merge-time diagnostics (#253), +// surfaced on every prototype deploy AND every successful prototype update +// (#259) so --prototype demo mode actually exercises both multi-warning +// paths in actionDoneMsg (app.go): the auto-open warnings overlay (deploy) +// and the update-results overlay's trailing warnings section (update batch - +// applyUpdatesSequentially). Same rationale as prototypeAllSourcesWarning +// (service.go): without these, no prototype mutation ever crosses +// formatOutcomeStatus's "> 1" collapse threshold, leaving those states +// unreachable in the one mode meant to demo every UI state. The strings +// exist purely to exercise the rendering paths; they name assets no canned +// mod actually bundles. Deliberately IDENTICAL on every call - like the real +// profile-level merge diagnostics a per-update recompile re-emits verbatim - +// so a multi-update prototype batch also demos the section's exact-text +// dedupe (one section, not one copy per mod). Returns a fresh slice per +// call so no caller ever aliases another outcome's Warnings. +func prototypeMergeWarnings() []string { + return []string{ + `asset "textures/armor/steel.dds" is bundled by both SkyUI and Ordinator - Ordinator wins (last-applied, per profile load order)`, + `asset "textures/armor/steel_n.dds" is bundled by both SkyUI and Ordinator - Ordinator wins (last-applied, per profile load order)`, + } +} + func (p *prototypeProvider) DeployProfile(_ context.Context) (ActionOutcome, error) { deployed := 0 for _, mod := range p.activeMods() { @@ -457,19 +482,7 @@ func (p *prototypeProvider) DeployProfile(_ context.Context) (ActionOutcome, err deployed++ } } - // Canned merge-time diagnostics (#253), surfaced on every prototype - // deploy so --prototype demo mode actually exercises the multi-warning - // auto-open overlay path (actionDoneMsg, app.go) - the same rationale as - // prototypeAllSourcesWarning (service.go): without these, no prototype - // mutation ever crosses formatOutcomeStatus's "> 1" collapse threshold, - // leaving the overlay unreachable in the one mode meant to demo every UI - // state. Like that constant, the strings exist purely to exercise the - // rendering path; they name assets no canned mod actually bundles. - warnings := []string{ - `asset "textures/armor/steel.dds" is bundled by both SkyUI and Ordinator - Ordinator wins (last-applied, per profile load order)`, - `asset "textures/armor/steel_n.dds" is bundled by both SkyUI and Ordinator - Ordinator wins (last-applied, per profile load order)`, - } - return ActionOutcome{Message: fmt.Sprintf("Deployed %d mod(s)", deployed), Warnings: warnings}, nil + return ActionOutcome{Message: fmt.Sprintf("Deployed %d mod(s)", deployed), Warnings: prototypeMergeWarnings()}, nil } // activeProfileName returns the canned Profiles entry currently marked @@ -784,7 +797,9 @@ func (p *prototypeProvider) CheckUpdates(_ context.Context) (UpdatesView, error) // ApplyUpdate emits the brief's own fake progress sequence, then bumps the // matching InstalledMods entry's Version to u.ToVersion and clears its // AvailableVersion - so a repeated CheckUpdates no longer reports it, -// mirroring a real update's "already up to date" outcome. +// mirroring a real update's "already up to date" outcome. The canned merge +// warnings demo #259's results-overlay warnings section - see +// prototypeMergeWarnings' doc comment. func (p *prototypeProvider) ApplyUpdate(_ context.Context, u UpdateItem, progress func(ActionProgress)) (ActionOutcome, error) { idx := p.findInstalledIndex(u.Source, u.ID) if idx < 0 { @@ -798,7 +813,7 @@ func (p *prototypeProvider) ApplyUpdate(_ context.Context, u UpdateItem, progres mods[idx].AvailableVersion = "" mods[idx].Status = "installed" - return ActionOutcome{Message: fmt.Sprintf("Updated %q to %s", u.Name, u.ToVersion)}, nil + return ActionOutcome{Message: fmt.Sprintf("Updated %q to %s", u.Name, u.ToVersion), Warnings: prototypeMergeWarnings()}, nil } // isValidUpdatePolicy reports whether policy is one of the three strings diff --git a/internal/tui/actions_provider_test.go b/internal/tui/actions_provider_test.go index 2d21443..a013131 100644 --- a/internal/tui/actions_provider_test.go +++ b/internal/tui/actions_provider_test.go @@ -532,6 +532,35 @@ func TestPrototypeProviderActions_ApplyUpdate_UnknownModErrors(t *testing.T) { assert.Error(t, err) } +// TestPrototypeProviderActions_ApplyUpdate_CannedMultiWarningDemo guards +// #259's demo-mode parity, mirroring +// TestPrototypeProviderActions_DeployProfile_CannedMultiWarningDemo (#253): a +// successful prototype update must return MORE than one canned merge warning +// so --prototype exercises the update-results overlay's trailing warnings +// section (applyUpdatesSequentially) - and the SAME canned text on every +// update, so a multi-update batch demos the exact-text dedupe too (one +// section, not one copy per mod - exactly how a real per-update recompile +// repeats the profile-level merge diagnostics). +func TestPrototypeProviderActions_ApplyUpdate_CannedMultiWarningDemo(t *testing.T) { + t.Parallel() + + actions := NewPrototypeProvider().(ActionProvider) + + view, err := actions.CheckUpdates(context.Background()) + require.NoError(t, err) + require.GreaterOrEqual(t, len(view.Updates), 2, "the canned set has two available updates (see prototype/data.go)") + + first, err := actions.ApplyUpdate(context.Background(), view.Updates[0], nil) + require.NoError(t, err) + assert.Greater(t, len(first.Warnings), 1, + "the prototype update must demo the multi-warning results section") + + second, err := actions.ApplyUpdate(context.Background(), view.Updates[1], nil) + require.NoError(t, err) + assert.Equal(t, first.Warnings, second.Warnings, + "identical canned text per update, so a batch demos exact-text dedupe") +} + // TestPrototypeRollbackSwapsVersions covers Task 6's rollback demo: the // canned "skse-address-library" InstalledMods entry (see prototype/data.go's // PreviousVersion doc comment) has Version "11"/PreviousVersion "10" - @@ -916,6 +945,12 @@ type recordingActions struct { // mid-batch update failure (one mod in a multi-update apply fails, // others succeed) without needing per-call outcome sequencing. ApplyUpdateErrByID map[string]error + + // ApplyUpdateOutcomeByID, if set, overrides ApplyUpdateOutcome for a + // specific UpdateItem.ID's successful return - lets a #259 test give + // each update in a batch its own Warnings without per-call outcome + // sequencing. ApplyUpdateErrByID still wins for an ID present in both. + ApplyUpdateOutcomeByID map[string]ActionOutcome } func (r *recordingActions) EnableMod(_ context.Context, item ModItem) (ActionOutcome, error) { @@ -983,6 +1018,9 @@ func (r *recordingActions) ApplyUpdate(_ context.Context, u UpdateItem, progress if err, ok := r.ApplyUpdateErrByID[u.ID]; ok { return ActionOutcome{}, err } + if out, ok := r.ApplyUpdateOutcomeByID[u.ID]; ok { + return out, nil + } return r.ApplyUpdateOutcome, r.ApplyUpdateErr } diff --git a/internal/tui/actions_test.go b/internal/tui/actions_test.go index 1858641..848a262 100644 --- a/internal/tui/actions_test.go +++ b/internal/tui/actions_test.go @@ -406,7 +406,12 @@ func TestActionDoneMultiWarningOutcomeReplacesStaleReadOnlyOverlay(t *testing.T) // priority when an outcome carries BOTH ResultLines and 2+ Warnings (today // only the apply-updates batch can): the pre-existing "update results" // overlay wins, and the warnings overlay defers rather than clobbering the -// batch's per-item record. +// batch's per-item record. This deferral is lossless since #259: the batch +// embeds its success-emitted warnings inside ResultLines as a trailing +// section (applyUpdatesSequentially), so the winning overlay already carries +// them - this test's hand-built outcome deliberately omits that section +// because the priority decision is what's pinned here, not the lines' +// content. func TestActionDoneResultLinesKeepPriorityOverWarningsOverlay(t *testing.T) { t.Parallel() diff --git a/internal/tui/app.go b/internal/tui/app.go index 79d2de9..3ed833b 100644 --- a/internal/tui/app.go +++ b/internal/tui/app.go @@ -620,6 +620,10 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // gate on m.action.running - the Files overlay is the reachable // case), and deferring to it would silently re-lose the warnings // (Copilot PR #258 finding), so a stale overlay is replaced instead. + // This deferral loses nothing (#259): the update batch embeds its + // success-emitted warnings INSIDE ResultLines as a trailing section + // (applyUpdatesSequentially, mutations.go), so the overlay that wins + // already carries them. if len(msg.outcome.Warnings) > 1 && !openedResultsOverlay { m.overlay = &infoOverlay{title: "warnings", lines: msg.outcome.Warnings} } diff --git a/internal/tui/mutations.go b/internal/tui/mutations.go index 3a07d1c..909af3f 100644 --- a/internal/tui/mutations.go +++ b/internal/tui/mutations.go @@ -2297,10 +2297,29 @@ func (m Model) resolveChangelogPicked(msg changelogPickedMsg) (Model, tea.Cmd) { // behavior just above: those mods never ran, which isn't the same thing as // failing). app.go's actionDoneMsg handler renders these as a scrollable // info overlay titled "update results" once the batch resolves. +// +// #259: warnings emitted by SUCCESSFUL updates are appended to ResultLines +// as one trailing section - a blank separator, then one line per distinct +// warning - because the "update results" overlay keeps priority over #253's +// warnings overlay (actionDoneMsg's openedResultsOverlay deferral), so any +// warning NOT inside ResultLines would be unreadable: a success's own entry +// is just its ✓ line. The section holds success-emitted warnings only - a +// failure's synthesized ": " warning already has its ✗ line in +// the same overlay and would render twice. These warnings are deduped on +// EXACT text (first occurrence wins, batch order) in both the section and +// the aggregate Warnings slice - keeping formatOutcomeStatus's "(N +// warnings)" count in step with the section - because they are mostly +// profile-LEVEL merge diagnostics ("asset X is bundled by both A and B"), +// re-emitted verbatim by every update that re-runs the merge, not facts +// about any one update; exact-match only, so distinct warnings that merely +// share a prefix never collapse into each other. Failure warnings are never +// deduped (their name prefix makes them distinct anyway). func applyUpdatesSequentially(ctx context.Context, actions ActionProvider, updates []UpdateItem, progress func(ActionProgress)) (ActionOutcome, error) { applied := 0 var warnings []string var resultLines []string + var successWarnings []string + seenSuccessWarnings := map[string]bool{} for _, u := range updates { if ctx.Err() != nil { break @@ -2325,9 +2344,20 @@ func applyUpdatesSequentially(ctx context.Context, actions ActionProvider, updat continue } applied++ - warnings = append(warnings, outcome.Warnings...) + for _, w := range outcome.Warnings { + if seenSuccessWarnings[w] { + continue + } + seenSuccessWarnings[w] = true + warnings = append(warnings, w) + successWarnings = append(successWarnings, w) + } resultLines = append(resultLines, fmt.Sprintf("✓ %s %s", u.Name, u.VersionLabel())) } + if len(successWarnings) > 0 { + resultLines = append(resultLines, "") + resultLines = append(resultLines, successWarnings...) + } return ActionOutcome{ Message: fmt.Sprintf("Applied %d update(s)", applied), Warnings: warnings, diff --git a/internal/tui/mutations_test.go b/internal/tui/mutations_test.go index ec9a32b..7ad0ece 100644 --- a/internal/tui/mutations_test.go +++ b/internal/tui/mutations_test.go @@ -2652,6 +2652,142 @@ func TestApplyUpdatesSequentiallyResultLines_CtxCancelledSkipsRemainder(t *testi require.Equal(t, []string{"✓ SkyUI 5.2 → 5.3"}, outcome.ResultLines) } +// --- #259: success-emitted warnings must be readable inside the results overlay --- + +// TestApplyUpdatesSequentially_SuccessWarningsAppendedAsSection guards #259's +// fix. Warnings emitted by SUCCESSFUL updates (on a merged-pak game, the +// merge-time asset-conflict diagnostics a recompile surfaces) used to be +// folded into the aggregate Warnings slice only - unreadable, because the +// "update results" overlay keeps priority over the warnings overlay (pinned +// by TestActionDoneResultLinesKeepPriorityOverWarningsOverlay) and a success's +// ResultLines entry is just its ✓ line. The batch must instead append them to +// ResultLines as one trailing section - a blank separator, then one line per +// distinct warning - so the ONE overlay the actionDoneMsg handler opens +// carries them. +func TestApplyUpdatesSequentially_SuccessWarningsAppendedAsSection(t *testing.T) { + t.Parallel() + + updates := []UpdateItem{ + {Source: "nexusmods", ID: "skyui", Name: "SkyUI", FromVersion: "5.2", ToVersion: "5.3"}, + {Source: "nexusmods", ID: "ussep", Name: "USSEP", FromVersion: "4.3", ToVersion: "4.4"}, + } + rec := &recordingActions{ + UpdatesViewOut: UpdatesView{Updates: updates}, + ApplyUpdateOutcomeByID: map[string]ActionOutcome{ + "skyui": {Message: `Updated "SkyUI" to 5.3`, Warnings: []string{ + `asset "a.uasset" is bundled by both SkyUI and Ordinator`, + `asset "b.uasset" is bundled by both SkyUI and Ordinator`, + }}, + "ussep": {Message: `Updated "USSEP" to 4.4`, Warnings: []string{ + `asset "c.uasset" is bundled by both USSEP and Ordinator`, + }}, + }, + } + + outcome, err := applyUpdatesSequentially(context.Background(), rec, updates, nil) + require.NoError(t, err) + require.Equal(t, []string{ + "✓ SkyUI 5.2 → 5.3", + "✓ USSEP 4.3 → 4.4", + "", + `asset "a.uasset" is bundled by both SkyUI and Ordinator`, + `asset "b.uasset" is bundled by both SkyUI and Ordinator`, + `asset "c.uasset" is bundled by both USSEP and Ordinator`, + }, outcome.ResultLines) + require.Equal(t, []string{ + `asset "a.uasset" is bundled by both SkyUI and Ordinator`, + `asset "b.uasset" is bundled by both SkyUI and Ordinator`, + `asset "c.uasset" is bundled by both USSEP and Ordinator`, + }, outcome.Warnings, "the aggregate Warnings must keep the status line's '(N warnings)' count in step with the section") +} + +// TestApplyUpdatesSequentially_FailuresAndSuccessWarningsBothReadable (#259): +// a batch carrying BOTH ✗ failures and success-emitted warnings must render +// both in ResultLines, without duplicating the failure text - a failure's +// synthesized ": " warning stays OUT of the trailing section +// (which holds success-emitted warnings only), because the failure already +// has its own ✗ line in the same overlay. +func TestApplyUpdatesSequentially_FailuresAndSuccessWarningsBothReadable(t *testing.T) { + t.Parallel() + + updates := []UpdateItem{ + {Source: "nexusmods", ID: "skyui", Name: "SkyUI", FromVersion: "5.2", ToVersion: "5.3"}, + {Source: "nexusmods", ID: "ussep", Name: "USSEP", FromVersion: "4.3", ToVersion: "4.4"}, + } + rec := &recordingActions{ + UpdatesViewOut: UpdatesView{Updates: updates}, + ApplyUpdateOutcomeByID: map[string]ActionOutcome{ + "skyui": {Message: `Updated "SkyUI" to 5.3`, Warnings: []string{ + `asset "a.uasset" is bundled by both SkyUI and Ordinator`, + `asset "b.uasset" is bundled by both SkyUI and Ordinator`, + }}, + }, + ApplyUpdateErrByID: map[string]error{"ussep": errors.New("connection refused")}, + } + + outcome, err := applyUpdatesSequentially(context.Background(), rec, updates, nil) + require.NoError(t, err) + require.Equal(t, []string{ + "✓ SkyUI 5.2 → 5.3", + "✗ USSEP: connection refused", + "", + `asset "a.uasset" is bundled by both SkyUI and Ordinator`, + `asset "b.uasset" is bundled by both SkyUI and Ordinator`, + }, outcome.ResultLines) + require.Equal(t, []string{ + `asset "a.uasset" is bundled by both SkyUI and Ordinator`, + `asset "b.uasset" is bundled by both SkyUI and Ordinator`, + "USSEP: connection refused", + }, outcome.Warnings, "the failure still reaches the aggregate Warnings exactly once, in batch order") +} + +// TestApplyUpdatesSequentially_SuccessWarningsDedupedOnExactText (#259): a +// multi-update batch on a merged-pak game re-runs the profile merge once per +// update, so the SAME profile-level conflict text repeats once per successful +// update. Dedupe on EXACT text (never a prefix or fuzzy match - a +// genuinely-distinct warning must never collapse into an unrelated one) keeps +// one line per distinct warning in first-occurrence order, in BOTH the +// trailing section and the aggregate Warnings - so the status line's count +// matches what the overlay shows. +func TestApplyUpdatesSequentially_SuccessWarningsDedupedOnExactText(t *testing.T) { + t.Parallel() + + updates := []UpdateItem{ + {Source: "nexusmods", ID: "skyui", Name: "SkyUI", FromVersion: "5.2", ToVersion: "5.3"}, + {Source: "nexusmods", ID: "ussep", Name: "USSEP", FromVersion: "4.3", ToVersion: "4.4"}, + } + sharedA := `asset "a.uasset" is bundled by both SkyUI and Ordinator` + rec := &recordingActions{ + UpdatesViewOut: UpdatesView{Updates: updates}, + ApplyUpdateOutcomeByID: map[string]ActionOutcome{ + "skyui": {Message: `Updated "SkyUI" to 5.3`, Warnings: []string{ + sharedA, + `asset "a.uasset" is bundled by both SkyUI and Requiem`, + }}, + "ussep": {Message: `Updated "USSEP" to 4.4`, Warnings: []string{ + sharedA, + `asset "c.uasset" is bundled by both USSEP and Ordinator`, + }}, + }, + } + + outcome, err := applyUpdatesSequentially(context.Background(), rec, updates, nil) + require.NoError(t, err) + require.Equal(t, []string{ + "✓ SkyUI 5.2 → 5.3", + "✓ USSEP 4.3 → 4.4", + "", + sharedA, + `asset "a.uasset" is bundled by both SkyUI and Requiem`, + `asset "c.uasset" is bundled by both USSEP and Ordinator`, + }, outcome.ResultLines, "the repeated warning collapses to one line; near-identical text (same prefix, different tail) must NOT collapse") + require.Equal(t, []string{ + sharedA, + `asset "a.uasset" is bundled by both SkyUI and Requiem`, + `asset "c.uasset" is bundled by both USSEP and Ordinator`, + }, outcome.Warnings) +} + // TestActionDoneOpensUpdateResultsOverlay is the end-to-end happy path: // confirming the apply-updates batch, once it resolves, opens a scrollable // info overlay titled "update results" listing each update's own outcome @@ -3038,7 +3174,21 @@ func TestPrototypeUpdatesEndToEndKeyFlow(t *testing.T) { updated, refreshCmd := model.Update(doneMsg) model = updated.(Model) require.NotNil(t, refreshCmd) - require.Equal(t, "Applied 2 update(s)", model.action.status) + // #259: each canned prototype update emits the same two merge warnings + // (prototypeMergeWarnings), deduped across the batch to exactly two - + // counted on the one-row status line, readable in full inside the + // results overlay's trailing section below. + require.Equal(t, "Applied 2 update(s) (2 warnings)", model.action.status) + require.NotContains(t, model.action.status, "\n", "status must stay one line") + require.NotNil(t, model.overlay) + require.Equal(t, "update results", model.overlay.title) + require.Equal(t, []string{ + "✓ SkyUI 5.2 → 5.3", + "✓ USSEP 4.3 → 4.4", + "", + `asset "textures/armor/steel.dds" is bundled by both SkyUI and Ordinator - Ordinator wins (last-applied, per profile load order)`, + `asset "textures/armor/steel_n.dds" is bundled by both SkyUI and Ordinator - Ordinator wins (last-applied, per profile load order)`, + }, model.overlay.lines, "the per-mod record plus ONE deduped warnings section - not one copy per update") loadedMsg := refreshCmd() require.IsType(t, dataLoadedMsg{}, loadedMsg, "the updates flow never triggers the install-only search-refresh batching")