Skip to content

Fix 17 code-review defects (2026-09-25): packaging, NPEs, CI, docs - #15

Merged
perNyfelt merged 25 commits into
mainfrom
review-fixes-2026-09-25
Sep 25, 2026
Merged

perNyfelt merged 25 commits into
mainfrom
review-fixes-2026-09-25

Conversation

@perNyfelt

Copy link
Copy Markdown
Member

Summary

Implements req/2026-09-25-fixes.md, the 15-task plan closing all 17 findings from the 2026-09-25 code review of gi-common, gi-swing, gi-fx, and gi-console, plus a final-review fix pass that caught two regressions the plan itself introduced.

Modules touched: gi-common, gi-swing, gi-fx, gi-console, buildSrc, root build/CI/release tooling.

Plan tasks (1-15)

  • Publish gi-common at compile scope from gi-swing/gi-console (was runtime-scoped, broke Java consumers)
  • Fat jars rebuilt on GradleUp Shadow with mergeServiceFiles() (a plain Jar + DuplicatesStrategy.EXCLUDE was silently dropping most META-INF/services registrations from ph-css/gsvg/Tika)
  • CI now actually runs Spotless/SpotBugs (org.gradle.configuration-cache=true was silently skipping both); dependency-check job's triggers are now declared so its if: is reachable
  • Swing/FX null-safety: chooser initial directory, SVG title, clipboard empty-list, ragged table rows/columns, escaping IOException from view(File), SVG detection fallback by filename
  • Console: Desktop.Action.OPEN check before opening a file, null-file handling, stdin charset decoding, dropped a stray println("Lost ownership")
  • JavaFX Viewer.viewTable tolerates ragged rows / short column-type lists via a new JavaFX-free TableData helper
  • release.sh verifies the version was actually written to build.gradle and reports partial-publish state instead of failing silently
  • Sample scripts (test.gsh, test.groovy) updated to current versions/package names
  • Full changelog in release.md (Unreleased section) and TODO.md

Final-review fix pass

A fresh review (dispatched on the most capable model, range df1e3c9..9fa3fc4) found two Critical regressions the plan's own Task 2 introduced, plus three Important gaps. All are fixed here, each verified RED→GREEN:

  • Critical: the Shadow migration declared the fatjar artifact twice (once explicitly, once via Shadow's own publish variant), so gradlew publish failed for gi-console/gi-fx/gi-swing with "multiple artifacts with the identical extension and classifier." Verified via publishToMavenLocal against a scratch repo.
  • Critical: DuplicatesStrategy.INCLUDE left each bundled jar's META-INF/groovy/...ExtensionModule unmerged, so the classloader silently picked only one jar's Groovy extensions (e.g. matrix-core's Row/Column/Pipe extensions disappeared behind matrix-groovy-ext's). Fixed with mergeGroovyExtensionModules(), verified via a URLClassLoader probe against the rebuilt fat jar.
  • Important: stdinCharset() was a no-op on every currently-supported JDK (stdin.encoding only exists from JDK 25) — added System.console()?.charset() as a JDK 17+ intermediate fallback.
  • Important: the partial-release recovery procedure in PUBLISHING.md described a flow that doesn't actually occur (no tag exists yet at that point, and Central rejects re-publishing an already-shipped module). Added release.sh --skip <module,...> and rewrote the doc to match.
  • Important: display(Svg) still NPE'd on a null svg in both Swing and FX — the renderer dereferenced the svg before the already-null-safe svgTitle helper ran. Added an isDisplayable(Svg) guard checked first in both modules.

Six Minor findings were deliberately left unfixed and are listed in the session's implementation notes for a follow-up pass (console display(String) null handling, a cosmetic Swing dialog title, .svg-by-name overriding confident content-type detection, an incomplete test.groovy sample fix, a pre-existing Swing alignment edge case, and TableData's visibility).

Test plan

  • ./gradlew clean test --console=plain — BUILD SUCCESSFUL
  • ./gradlew check --no-configuration-cache --console=plain — BUILD SUCCESSFUL (Spotless + SpotBugs)
  • publishToMavenLocal for all four modules against a scratch repo — succeeds, each fatjar present with correct classifier and compile-scoped gi-common dependency
  • URLClassLoader probe against the rebuilt gi-console fatjar — all four Groovy extension classes present in one merged descriptor
  • ./release.sh --dry-run and ./release.sh --dry-run --skip gi-common,gi-console — both produce the expected step list with no working-tree mutation

🤖 Generated with Claude Code

perNyfelt and others added 20 commits September 25, 2026 17:44
Co-Authored-By: Kimi <noreply@moonshot.cn>
Co-Authored-By: Kimi <noreply@moonshot.cn>
Co-Authored-By: Kimi <noreply@moonshot.cn>
Co-Authored-By: Kimi <noreply@moonshot.cn>
Co-Authored-By: Kimi <noreply@moonshot.cn>
Co-Authored-By: Kimi <noreply@moonshot.cn>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Shadow already registers the shadowJar output as a publication variant
once the plugin is applied; the explicit artifact(tasks.named('shadowJar'))
declared a second artifact with the same extension and classifier,
which Gradle rejects when publishing. Verified: gi-common, gi-console,
gi-fx, and gi-swing all now publish to a scratch Maven repo, each still
producing its <module>-<version>-fatjar.jar.

Found in final review of req/2026-09-25-fixes.md.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
With DuplicatesStrategy.INCLUDE and no ExtensionModule merge, each
bundled jar's META-INF/groovy/org.codehaus.groovy.runtime.ExtensionModule
was written as an unmerged duplicate, and the JVM's classloader resolved
only one of them -- silently hiding, e.g., matrix-core's Row/Column/Pipe
extensions behind matrix-groovy-ext's NumberExtension in the gi-console
fat jar. mergeGroovyExtensionModules() unions them the same way
mergeServiceFiles() unions META-INF/services/*. Verified via a
URLClassLoader probe against the rebuilt fat jar: all four extension
classes are now present in one merged descriptor.

Found in final review of req/2026-09-25-fixes.md.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
stdin.encoding is only reported from JDK 25 onward, so on this
project's JDK 21 toolchain stdinCharset() always fell straight through
to Charset.defaultCharset() -- the fix did nothing on any currently
supported JDK. Added System.console()?.charset() as an intermediate
fallback (available since JDK 17, reflects the platform's actual
console encoding), split into a testable resolveCharset(property,
consoleCharset) so the fallback order can be exercised without a live
console.

Found in final review of req/2026-09-25-fixes.md.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The documented recovery procedure ("re-run with the same version, the
tag check will warn, answer y") does not match the script: at that
point no tag exists yet (it's created only after every module
publishes), and Maven Central rejects re-publishing an already-shipped
module/version, so a bare re-run fails again on the first module.
--skip gi-common,gi-console lets a re-run skip modules a prior partial
run already published. Verified with --dry-run --skip.

Found in final review of req/2026-09-25-fixes.md.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
svgTitle(null) was already null-safe, but the null check ran too late:
Swing's SvgRenderer.toBufferedImage(chart) and FX's
ChartToJfx.export(svg) both dereference the svg first and throw before
svgTitle is ever reached, so display(Svg) still NPE'd on a null
argument despite Task 7's fix. Added isDisplayable(Svg) to both
modules' InOut and check it before rendering, matching how every
other display/view overload in these classes already treats a null
argument as "log and return" rather than throw.

Found in final review of req/2026-09-25-fixes.md.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 25, 2026 19:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Five unresolved moderate findings remain in JavaFX table handling, Swing SVG detection, and release recovery diagnostics.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3 Medium severity

Open (3)
What changed in this PR

This PR fixes packaging, null-safety, CI verification, release tooling, documentation, and sample scripts across the project.

Changes:

  • Migrates fat JAR packaging to Shadow with metadata merging.
  • Adds defensive handling and focused tests across Swing, JavaFX, console, and clipboard functionality.
  • Updates release recovery, CI checks, documentation, and examples.
File Summary
TODO.md Records completed review fixes.
test.groovy Updates Matrix package examples.
release.sh Adds version checks and partial-release skipping.
release.md Documents fixes and packaging changes.
README.md Documents verification commands.
gi-swing/​test.gsh Updates the released dependency version.
gi-swing/​src/​test/​groovy/​se/​alipsa/​gi/​swing/​InOutHelperTest.groovy Tests Swing helpers.
gi-swing/​src/​main/​groovy/​se/​alipsa/​gi/​swing/​InOut.groovy Improves Swing null, table, SVG, and file handling.
gi-swing/​build.gradle Publishes the common API dependency.
gi-fx/​test.gsh Updates the released dependency version.
gi-fx/​src/​test/​groovy/​se/​alipsa/​gi/​fx/​TableDataTest.groovy Tests safe table access.
gi-fx/​src/​test/​groovy/​se/​alipsa/​gi/​fx/​InOutHelperTest.groovy Tests JavaFX helpers.
gi-fx/​src/​main/​groovy/​se/​alipsa/​gi/​fx/​Viewer.groovy Handles missing table cells and types.
gi-fx/​src/​main/​groovy/​se/​alipsa/​gi/​fx/​TableData.groovy Provides bounds-safe table accessors.
gi-fx/​src/​main/​groovy/​se/​alipsa/​gi/​fx/​InOut.groovy Improves chooser and SVG handling.
gi-fx/​build.gradle Configures fat JAR publication.
gi-console/​test.gsh Updates the released dependency version.
gi-console/​src/​test/​groovy/​se/​alipsa/​gi/​console/​ConsolePromptTest.groovy Tests console behavior and charset handling.
gi-console/​src/​main/​groovy/​se/​alipsa/​gi/​console/​InOut.groovy Improves desktop, stdin, and clipboard handling.
gi-console/​build.gradle Exposes the common API dependency.
gi-common/​src/​test/​groovy/​se/​alipsa/​gi/​AbstractInOutTest.groovy Tests empty clipboard handling.
gi-common/​src/​main/​groovy/​se/​alipsa/​gi/​GuiInteraction.groovy Updates API documentation.
gi-common/​src/​main/​groovy/​se/​alipsa/​gi/​AbstractInOut.groovy Safely handles clipboard contents.
docs/​PUBLISHING.md Documents partial-release recovery.
docs/​DEPENDENCIES.md Documents dependency scopes and fat JARs.
docs/​API-Guide.md Documents updated behavior.
buildSrc/​src/​main/​groovy/​se.alipsa.gi.fatjar-conventions.gradle Configures Shadow packaging and descriptor merging.
buildSrc/​build.gradle Adds the Shadow plugin dependency.
AGENTS.md Updates packaging and sample-script guidance.
.github/​workflows/​ci.yml Enables static analysis and scheduled checks.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread release.sh Resolved
@perNyfelt
perNyfelt merged commit 421953a into main Sep 25, 2026
2 checks passed
@perNyfelt
perNyfelt deleted the review-fixes-2026-09-25 branch September 27, 2026 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants