fix(reporter): pre-fill the GitHub issue form with a crash summary - #58
Open
soloturn wants to merge 6 commits into
Open
fix(reporter): pre-fill the GitHub issue form with a crash summary#58soloturn wants to merge 6 commits into
soloturn wants to merge 6 commits into
Conversation
'Report Issue' just opened REPORT_ISSUE_LINK as-is - a bare https://github.com/.../issues/new, always blank, discarding everything the dialog already knows about the crash. Adds CrashSummary, which builds a title (exception class + message) and a Markdown body from three sources: - the exception itself (available directly, no parsing needed) - class, message, and a capped stack trace extract - the engine version and active module list, which only exist in the crashed process' own log output (the reporter runs in its own JVM per #52's subprocess isolation, so it has no other way to reach them) - extracted via regex against two fixed log lines TerasologyEngine already emits at startup (TerasologyVersion's bracketed key=value dump, and one "Activating module: id:version" line per active module) - the OS, read directly via System.getProperty (same machine, same session as the crash - no parsing needed) - the uploaded PasteBin link, when the user uploaded one GitHubIssueLinkBuilder turns that into a real pre-filled URL via GitHub's own title=/body= query parameters. Both are plain, dependency- free classes - no Swing - so they're covered directly by CrashSummaryTest/GitHubIssueLinkBuilderTest without needing a headless UI harness. Second fix from #53 (item 3 of 5); log tab ordering, the dead forum link, and the Discord invite are still open follow-ups. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Aug 22, 2026
BenjaminAmos
pushed a commit
that referenced
this pull request
Aug 22, 2026
sortLogFiles() sorted by file creation time, reversed (newest first). With two log files from one session (Terasology-init.log, Terasology-menu.log) that reads as arbitrary - neither the order they were written in nor the order their names suggest - rather than a deliberate choice. Sorting by filename instead is deterministic and, for Terasology's own log naming, happens to match session order too. Also carries the GlobalProperties NPE guard from #56 (needed to construct GlobalProperties() at all in cr-core's own test classpath, same as there - see that PR for the full explanation). Will collapse to a no-op merge once #56 lands first. First item from #53 (1 of 5); items 2 and 3 are #56 and #58. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Rather than inventing a new template, can you use the existing one at https://github.com/MovingBlocks/Terasology/blob/develop/.github/ISSUE_TEMPLATE/crash-bug-report.md instead? If we convert that template into an issue form, then we might be able to do even better and just pre-populate certain fields (see https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/creating-an-issue#creating-an-issue-from-a-url-query). |
2 tasks
CrashSummary.buildBody() only ever showed the single in-process Throwable the reporter happened to be invoked with. Found while testing with multiple log tabs (init/menu/game): the game.log tab had its own NullPointerException, but "File an issue on GitHub" only pre-filled the crash that triggered the reporter, silently leaving the other one out even though ErrorMessagePanel#getLog() already combines every tab's text into what CrashSummary gets. New "### Other exceptions found in logs" section scans that combined text for stack traces (any "some.FullyQualified.NameException[: message]" header line immediately followed by "at "/"Caused by:" frames - the shape every JVM logging framework prints a Throwable in), attributes each to its tab, and skips the primary exception's own entry so it isn't listed twice when the crashed process also logged it in its own log file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2 tasks
Splitting this into a separate "Exception" block (the primary crash, full multi-line trace) and a completely separate "Other exceptions found in logs" section far below Environment made the two hard to connect and easy to miss - reported as confusing after testing live. Now every exception found - the primary one first, then every other one - is a single row directly under "### Exceptions", each naming the log tab it was found in (or "this crash" if it wasn't logged in any tab, which is normal - it's the in-process exception that triggered the report). That section comes immediately before "### Environment". The primary exception is attributed to whichever tab also logged it, instead of being duplicated as a separate "other" entry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous one-line-per-exception format was too terse to actually
debug from - a real fix needs the trace, not just a class name and
message. Every exception found now gets its own labeled code block
(full trace, capped at 15 lines like before) instead of a single-line
summary, still all together under "### Exceptions" before
"### Environment".
Also: on macOS the crash reporter always relaunches in a subprocess
(requiresProcessIsolation()), which reconstructs the exception from
just its class name and message - its own stack trace points into
CrashReporter's own relaunch machinery, not the real crash site. So
whenever the primary exception was also logged in one of the log tabs
(the normal case for an engine-level crash handler), the trace
captured from that log text is used instead - it's the real one. The
exception object's own trace is now only a fallback for when nothing
better is available.
Fixed a real bug found while writing this: collectExceptionHeaders()
and framesFromThrowable() both called String.trim() on the captured
frame text, which silently ate the leading tab off the *first* frame
line ("\tat ...") since trim() only strips from the very edges of the
whole string, not per-line - every trace in the pre-filled issue would
have rendered its first frame without the indentation the rest have.
New stripTrailingWhitespace() strips only the trailing newline.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tion Requested after testing: knowing only the exception itself often isn't enough to understand what led up to it. Each exception block now starts with up to 5 lines of whatever was logged immediately before it in that tab, then the exception's own header and trace as before. The in-process fallback trace (see the previous commit's javadoc on macOS reconstruction) has no log text to pull context from, so it's unaffected. Context is not subject to the trace's own 15-line cap - a few lines of what led up to the crash shouldn't cost trace detail. Found and fixed a boundary bug while writing this: the substring for each log tab started right after "=== tab ===", before that line's own newline, so every tab's text began with a blank artifact line - counted as a real (empty) line of context. Tab boundaries now skip past that line terminator, so the 5 lines captured are always real log content. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Aug 22, 2026
Contributor
Author
|
Good idea - opened MovingBlocks/Terasology#5390 converting crash-bug-report.md to an issue form (same sections/fields, individually addressable by ID). Once that's in, I'll rework this PR's CrashSummary to pre-fill it via the field-ID query params instead of the custom body it builds now. |
…stom body @BenjaminAmos noted "Report Issue" should use the repo's existing crash-bug-report template rather than a bespoke Markdown format, and suggested converting it to a GitHub issue form so individual fields could be pre-populated by ID via URL query (MovingBlocks/Terasology#5390 does that conversion). New GlobalProperties.KEY.REPORT_ISSUE_TEMPLATE: when a downstream app sets it (cr-terasology now does, to "crash-bug-report.yml"), FinalActionsPanel builds a `template=`+per-field-ID query via a new GitHubIssueLinkBuilder.build(baseUrl, template, title, fields) overload and CrashSummary.buildIssueFormFields(), landing the crash summary in that form's real "Terasology Version"/"Operating System"/"Java Version"/"What actually happened"/"Log details"/"Additional Infos" fields instead of overwriting the whole issue with a custom body. Apps without a configured template (cr-destsol, standalone cr-core) keep the existing generic buildTitle()/buildBody() title+body fallback unchanged - REPORT_ISSUE_TEMPLATE is engine-agnostic cr-core plumbing, but the field IDs it targets when set are inherently tied to whichever form the downstream app's own repo defines, so this can never be the unconditional default. Also reapplied GitHubIssueLinkBuilder's null-baseUrl guard here (this branch predates that fix, added directly on merge-train earlier this session) - build() must not silently produce a broken "null?title=..." link when REPORT_ISSUE_LINK isn't configured. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
2 participants
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
"Report Issue" just opened
REPORT_ISSUE_LINKas-is - a barehttps://github.com/.../issues/new, always blank, discarding everything the dialog already knows about the crash.CrashSummaryextracts, from the exception and the crashed process' own log output (the reporter runs in its own JVM per #52's subprocess isolation, so there's no other way to reach engine-version/module info):Every exception found - one labeled block each (full trace, capped at 15 lines, plus the 5 log lines logged right before it), naming the log tab it was found in. The one that triggered the report is always first, attributed to whichever tab also logged it if any (otherwise labeled "this crash"); every other exception found across the log tabs follows, so a crash whose real cause is an earlier exception logged in a different tab (e.g. during init) isn't left out.
On macOS the crash reporter always relaunches in a subprocess (
requiresProcessIsolation()), which reconstructs the exception from just its class name and message - its own stack trace points into the reporter's own relaunch machinery, not the real crash site. Whenever the triggering exception was also logged in a tab, the trace captured from that log text is used instead, since it's the real one.Engine version + active modules - extracted via regex against two fixed lines
TerasologyEnginealready emits at startup.OS + Java version - read directly via
System.getProperty.The uploaded PasteBin link, when the user uploaded one.
How it reaches GitHub: @BenjaminAmos noted this shouldn't invent its own body format when Terasology already has a
crash-bug-reporttemplate, and suggested converting it to an issue form so fields could be pre-populated individually. MovingBlocks/Terasology#5390 does that conversion. So:GlobalProperties.KEY.REPORT_ISSUE_TEMPLATE- when a downstream app sets it (cr-terasologynow does, tocrash-bug-report.yml),CrashSummary.buildIssueFormFields()+ a newGitHubIssueLinkBuilder.build(baseUrl, template, title, fields)overload build atemplate=+per-field-ID query, landing the summary in that form's real "Terasology Version"/"Operating System"/"Java Version"/"What actually happened"/"Log details"/"Additional Infos" fields instead of overwriting the whole issue.cr-destsol, standalonecr-core) keep the original genericbuildTitle()/buildBody()title+body fallback unchanged - the field IDs a configured template targets are inherently tied to whichever form that specific downstream repo defines, so this can never becr-core's unconditional default.Both
CrashSummaryandGitHubIssueLinkBuilderare plain, dependency-free classes with no Swing dependency, so they're covered directly by unit tests without a headless UI harness.Second fix from #53 (item 3 of 5); log tab ordering, the dead forum link, and the Discord invite are still open follow-ups.
Test plan
CrashSummaryTest- version/display-version extraction, module dedup, graceful fallback, title formatting, exception blocks (full trace + context lines) for both the triggering exception and others found in other tabs, PasteBin link handling, andbuildIssueFormFields()'s per-field extraction/omission.GitHubIssueLinkBuilderTest- query-param encoding,nullwhenREPORT_ISSUE_LINKisn't configured, the template+fields overload's query building and its omission of empty fields../gradlew build- clean.crash-bug-report.ymlform fields (Terasology Version, Operating System, Java Version, What actually happened, etc.), not a blank/fallback issue.Related
cr-terasology).