feat: surface per-component conversion warnings in job results UI

[im-shiv]

BucketDataSource: parse path||message warning strings from bucket resource and inject warnings (boolean) + warningList (String[]) into each datasource item. item.html: render warning icon and message list for flagged items using HTL data-sly-test/data-sly-list. navpanel.html: remove unused multi-bucket loop, hardcode single Bucket 1 tab (forms jobs always run as a single bucket).

[iamsudhanshu]

Have you validated the impact on Site page migration?

[iamsudhanshu]

Please attach pre-post change screenshots for clarity on user experience.

[iamsudhanshu]

Review comments from Claude:
PR #242 — Code Review

feat: surface per-component conversion warnings in job results UI

Overview

Three changes as described: ConversionJobBucketDataSource parses path||message warning strings stored in the bucket JCR node (written by PR #240) and injects warnings (boolean) and warningList (String[]) into each datasource item's view
model; item.html renders a warning section per item; navpanel.html is described as having the multi-bucket loop removed, but that change is missing from the diff.

---

Critical Issue

1. navpanel.html change described but absent from the diff

The PR description says:

▎ navpanel.html: remove unused multi-bucket loop, hardcode single Bucket 1 tab (forms jobs always run as a single bucket).

The diff contains no changes to navpanel.html. The file still has the full data-sly-list.bucket="${job.navBuckets}" loop. Either the work is incomplete, or the description is inaccurate. This needs resolution before merge.

Beyond the incompleteness, the proposed change itself is architecturally risky: navpanel.html is shared across all job types (component, structure, full, form). Hardcoding "Bucket 1" would silently regress the multi-bucket tab view for
non-form jobs. The claim "forms jobs always run as a single bucket" may be true today, but it's an implicit contract that isn't enforced anywhere in the backend — this warrants either a code comment or a runtime guard, not a UI hardcode.

---

Issues

2. Inline styles should be CSS classes

⚠ ${warning}

The rest of item.html uses CSS classes (class="mediumIcon ${properties.statusClass}"), keeping styling out of the markup. This inline style:

• Bypasses any style-src Content Security Policy directives the AEM instance may enforce.
• Cannot be themed or overridden via the project's clientlibs.
• Hardcodes the Coral/Spectrum error red as a hex literal instead of using an existing --spectrum-global-color-red-600 token or a project-defined class.

The warning style should be a named class in the project's clientlib (e.g. aem-modernize-warning-message), consistent with how statusClass is used.

3. "warnings" is a cross-PR magic string with no shared constant

PR #240 writes:
mvm.put("warnings", bucketWarnings.toArray(new String[0]));
This PR reads:
bucket.getResource().getValueMap().get("warnings", new String[0]);

The JCR property name "warnings" is a literal string duplicated across two classes in two PRs with no shared constant. If it changes in one place, the other silently breaks. Define it as a constant in ConversionJobBucket (which already owns
the bucket model) and reference it in both places.

4. No tests for the new warning path

The existing testBucket verifies path/status/icon but doesn't cover any warning scenarios. No new tests were added for:

• A bucket resource with a warnings property → warnings=true and warningList populated correctly.
• An item in the bucket with no warnings → no warnings key in the view model.
• A warning entry with no || separator (malformed) → silently skipped (currently handled by sep > 0, but unverified).
• Multiple warnings for the same path → all accumulated into the list.

5. Silent discard of malformed warning entries

if (sep > 0) {
// parse it
}
// else: silently drop

Entries where sep <= 0 (no separator, or separator at position 0) are silently dropped with no log output. A logger.debug or logger.warn here would make debugging bad warning data much easier.

6. parseWarnings is coupled to the || delimiter defined in PR #240

The delimiter contract is implicit between two PRs with no shared documentation or constant. Consider at minimum a comment in parseWarnings pointing to where the format is produced, so future maintainers can find the other side of the
contract.

7. data-sly-list loop variable name shadows the boolean flag

properties.warnings (boolean flag) and the loop variable warning (individual message) are similar enough to cause confusion on a quick read. Consider renaming the loop variable to warningMsg or warningItem.

---

Minor / Non-blocking

• sep > 0 vs sep >= 0: The check sep > 0 requires the path to be non-empty (position > 0), which is correct behavior. The intent could be clearer with an explicit blank-path check and a comment.
• new String[0] idiom: Fine, but String[] can also be written as bucket.getResource().getValueMap().get("warnings", String[].class) which reads more naturally and also returns null instead of an empty array on miss — worth checking which
  semantics are preferred.
• HTL auto-escaping is sufficient: ${warning} in a text context is HTML-entity encoded by HTL by default, so there's no XSS risk from warning message content. No change needed, but ${warning @ context='text'} would make the intent explicit
  and is idiomatic in AEM HTL authoring.

[bstopp]

See PR review from Claude, provided by @iamsudhanshu