Merge pull request #265 from DonovanMods/dyoung522/fix-259-update-batch-warnings

fix(tui): render success-emitted batch-update warnings in the update-results overlay (#259)

Author: dyoung522

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 <method>` claim, each mod's `✓` line is labeled by how its content

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 "✓ <name> <from> → <to>" line per successful update,
 	// one "✗ <name>: <error>" 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,26 +453,36 @@ 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() {
 		if mod.Status != "disabled" {
 			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

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
 }
 

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()
 

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}
 		}

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 "<name>: <error>" 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,