Codex-generated pull request #20
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/pl/koder95/eme/App.java (1)
55-66:⚠️ Potential issue | 🟡 MinorAvoid double‑loading indices on startup.
IndexList::loadruns beforeapplicationContext.initialize()(which callscabinetAnalyzer.load()). If the analyzer loads indices too, this duplicates I/O and startup cost. Consider centralizing the load in one place or guarding to avoid duplicate work.src/main/java/pl/koder95/eme/fx/PersonalDataView.java (1)
106-116:⚠️ Potential issue | 🟠 MajorEnsure the progress dialog always closes and the thread doesn't keep the app alive.
While
reloadAll()handles checked exceptions internally (vialoadBooks()), unchecked exceptions (RuntimeException, NullPointerException) would prevent the dialog from closing. Additionally, the non-daemon thread can keep the JVM running after UI exit. Usetry-finallyto guarantee dialog closure on any exception, and setthread.setDaemon(true)to allow clean shutdown.🛠️ Suggested hardening
Dialog<Boolean> dialog = dialogs.createProgressDialog(scene, "Czekaj..."); Thread thread = new Thread(() -> { - indexReloadService.reloadAll(); - Platform.runLater(() -> { - dialog.setResult(true); - dialog.close(); - }); + try { + indexReloadService.reloadAll(); + } catch (Exception ex) { + // TODO: log or show error message + } finally { + Platform.runLater(() -> { + dialog.setResult(true); + dialog.close(); + }); + } }); +thread.setDaemon(true); dialog.setOnShown(event -> Platform.runLater(thread::start));
🤖 Fix all issues with AI agents
In `@src/main/java/pl/koder95/eme/application/IndexReloadService.java`:
- Around line 10-15: The reloadAll loop in IndexReloadService should guard each
index so one failure doesn't crash the thread or leave BOOKS null; wrap the body
that calls IndexList.clear() and IndexList.load() in a try-catch that catches
Exception (or IOException where applicable), logs the error with context
(including the IndexList value and exception), and continues to the next index
instead of rethrowing; update any progress/UI callbacks even on failure so the
progress dialog remains responsive; key symbols to modify are
IndexReloadService.reloadAll, IndexList.clear, IndexList.load, and the
loadBooks/BOOKS logic that currently throws NullPointerException.
In `@src/main/java/pl/koder95/eme/application/PersonalDataPresentation.java`:
- Around line 17-19: The fullName() method can throw NPE because it calls
surname.toUpperCase() without null checks; update
PersonalDataPresentation.fullName() to handle null or blank surname safely
(e.g., treat null surname as empty string or return name only), perform
null-safe checks for both name and surname, and use surname != null ?
surname.toUpperCase() : "" (or equivalent) when composing the result so calls
from PersonalDataQueryService.toPresentation() cannot NPE.
In `@src/main/java/pl/koder95/eme/dfs/FileXmlIndexDataSource.java`:
- Around line 34-49: The ensureFileExists() method currently checks
Files.exists(xmlPath) then opens Files.newBufferedWriter(xmlPath), which can
race if another process creates the file in between; change the creation logic
to attempt atomic creation (e.g. use Files.newOutputStream or
Files.newBufferedWriter with StandardOpenOption.CREATE_NEW or call
Files.createFile(xmlPath) and catch FileAlreadyExistsException) and only write
the default XML when the create succeeds; keep existing behavior when the file
already exists by catching FileAlreadyExistsException and returning, and ensure
xmlPath and the initial writer logic in ensureFileExists() are updated
accordingly.
In `@src/main/java/pl/koder95/eme/dfs/IndexList.java`:
- Around line 114-118: When loadBooks() calls INDEX_LOADER.loadBooks() and
catches IOException, BOOKS can remain null causing BOOKS.stream() to NPE; update
the catch block in loadBooks() to assign BOOKS = Collections.emptyList() (or new
ArrayList<>()) as a safe fallback and log the exception; ensure you reference
the static field BOOKS and the INDEX_LOADER.loadBooks() call so the assignment
happens inside the catch before the method returns.
In `@src/main/java/pl/koder95/eme/dfs/IndexLoader.java`:
- Around line 45-48: In IndexLoader, the code that constructs a Book from
bookNode assumes bookNode.getAttributes().getNamedItem("name") is non-null and
can throw an NPE; update the logic in the method that creates the Book (the
block referencing bookNode and new Book(...)) to first retrieve NamedNodeMap
attrs = bookNode.getAttributes(), then Node nameNode =
attrs.getNamedItem("name") and guard that nameNode != null before calling
getTextContent(); if nameNode is null, pass a safe default (e.g., empty string)
into new Book(...) or handle accordingly so no NPE occurs.
🧹 Nitpick comments (1)
src/main/java/pl/koder95/eme/dfs/Book.java (1)
53-66: Consider returning an unmodifiable view ofindices.Now that you’ve added
addIndex/addIndices, it may be safer to prevent external mutation viagetIndices()and makeindicesprivate, to keep changes funneled through the mutators.♻️ Suggested refactor
- final ObservableList<Index> indices; + private final ObservableList<Index> indices; @@ public ObservableList<Index> getIndices() { - return indices; + return FXCollections.unmodifiableObservableList(indices); }
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/pl/koder95/eme/fx/PersonalDataView.java (1)
101-117:⚠️ Potential issue | 🟡 MinorSet the reload thread as a daemon to allow JVM shutdown.
The reload thread is non-daemon; when
Platform.exit()is called (viaappCloseService.closeWithConfirmation()), it exits the JavaFX application but does not terminate the JVM. Non-daemon threads keep the JVM process alive, potentially causing the application to hang on exit if a reload is in progress.Fix
Dialog<Boolean> dialog = dialogs.createProgressDialog(scene, "Czekaj..."); Thread thread = new Thread(() -> { indexReloadService.reloadAll(); Platform.runLater(() -> { dialog.setResult(true); dialog.close(); }); }); + thread.setDaemon(true); dialog.setOnShown(event -> Platform.runLater(thread::start));
🤖 Fix all issues with AI agents
In `@src/main/java/pl/koder95/eme/bootstrap/ApplicationContext.java`:
- Around line 33-42: The ApplicationContext currently passes null for the
DataTarget to SimpleCabinetAnalyzer which leads to an NPE when
CabinetWorker.save() calls
dataTarget.setBaptism()/setConfirmation()/setMarriage()/setDecease(); fix this
by providing a no-op DataTarget implementation and use it in the
ApplicationContext constructor instead of null (or alternatively add a
null-check in CabinetWorker.save or validate non-null in SimpleCabinetAnalyzer's
constructor), e.g. implement a NoOpDataTarget that safely no-ops all setter
methods and inject that into new SimpleCabinetAnalyzer(...) so
CabinetWorker.save() can safely call the methods.
In `@src/main/java/pl/koder95/eme/dfs/IndexLoader.java`:
- Around line 42-53: parseBook currently throws IllegalArgumentException when a
<book> node lacks a name attribute, which aborts loading; change parseBook in
IndexLoader so it returns null instead of throwing when the name attribute is
missing or blank (use Optional.orElse(null) and StringUtils/trim check) and
optionally log a warning via the class logger; then update loadBooks (also in
IndexLoader) to skip null returns from parseBook (do not add them to the list)
so malformed <book> nodes are gracefully ignored at runtime (this preserves
behavior expected by IndexList which only catches IOException).
🧹 Nitpick comments (2)
src/main/java/pl/koder95/eme/application/IndexReloadService.java (1)
10-17: Add context + stack trace (and preserve interrupts) in reload failures.
System.err.println(ex)hides which index failed and drops the stack trace; if anInterruptedExceptionever bubbles up, the interrupt flag is lost. Consider logging the index and printing the stack trace while preserving interrupts.Suggested tweak
for (IndexList value : IndexList.values()) { try { value.clear(); value.load(); } catch (Exception ex) { - System.err.println(ex); + if (ex instanceof InterruptedException) { + Thread.currentThread().interrupt(); + } + System.err.println("Failed to reload index: " + value); + ex.printStackTrace(System.err); } }src/main/java/pl/koder95/eme/dfs/Book.java (1)
53-66: Consider returning an unmodifiable indices view.
getIndices()exposes the mutable list directly; callers can mutate it withoutaddIndex/addIndiceschecks. All current usages are read-only compatible, so returning an unmodifiable wrapper would enforce controlled mutations through the provided methods.♻️ Optional encapsulation
public ObservableList<Index> getIndices() { - return indices; + return FXCollections.unmodifiableObservableList(indices); }
| public ApplicationContext() { | ||
| this.cabinet = new TreeFilingCabinet(); | ||
| this.indexListDataSource = new IndexListDataSource(); | ||
| this.suggestionProvider = new SuggestionProvider(cabinet); | ||
| this.cabinetAnalyzer = new SimpleCabinetAnalyzer( | ||
| cabinet, | ||
| indexListDataSource, | ||
| null, | ||
| suggestionProvider | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "SimpleCabinetAnalyzer.java" -o -name "AbstractCabinetAnalyzer.java" -o -name "DataTarget.java" | head -20Repository: koder95/eMetrykant
Length of output: 245
🏁 Script executed:
cat -n ./src/main/java/pl/koder95/eme/core/spi/DataTarget.javaRepository: koder95/eMetrykant
Length of output: 1896
🏁 Script executed:
cat -n ./src/main/java/pl/koder95/eme/core/AbstractCabinetAnalyzer.javaRepository: koder95/eMetrykant
Length of output: 5392
🏁 Script executed:
cat -n ./src/main/java/pl/koder95/eme/core/AbstractCabinetWorker.javaRepository: koder95/eMetrykant
Length of output: 2625
🏁 Script executed:
cat -n ./src/main/java/pl/koder95/eme/core/SimpleCabinetAnalyzer.javaRepository: koder95/eMetrykant
Length of output: 2382
🏁 Script executed:
rg -n --type=java '\bgetDataTarget\(\)' -A 5Repository: koder95/eMetrykant
Length of output: 1966
🏁 Script executed:
cat -n ./src/main/java/pl/koder95/eme/core/spi/CabinetWorker.javaRepository: koder95/eMetrykant
Length of output: 3786
🏁 Script executed:
rg -n --type=java '\bsave\(\)' -B 2 -A 2Repository: koder95/eMetrykant
Length of output: 595
🏁 Script executed:
rg -n --type=java '\.save\(\)' -B 2 -A 2Repository: koder95/eMetrykant
Length of output: 44
🏁 Script executed:
rg -n --type=java 'save\(' -B 1 -A 1Repository: koder95/eMetrykant
Length of output: 321
🏁 Script executed:
rg -n --type=java 'cabinetAnalyzer\.save\|\.save\(\)' src/main/java/pl/koder95/eme/bootstrap/Repository: koder95/eMetrykant
Length of output: 44
🏁 Script executed:
cat -n ./src/main/java/pl/koder95/eme/bootstrap/ApplicationContext.javaRepository: koder95/eMetrykant
Length of output: 3170
🏁 Script executed:
rg -n --type=java 'cabinetAnalyzer|getDataTarget\(\)' src/main/java/pl/koder95/eme/application/ src/main/java/pl/koder95/eme/fx/ -A 2 -B 2Repository: koder95/eMetrykant
Length of output: 44
🏁 Script executed:
find src/main/java -name "*.java" -type f | xargs grep -l "\.save()" | head -20Repository: koder95/eMetrykant
Length of output: 44
🏁 Script executed:
rg -n --type=java 'save\(' src/main/java/ | grep -v 'datasource\|DataTarget'Repository: koder95/eMetrykant
Length of output: 147
🏁 Script executed:
find src -name "*.java" -type f | xargs grep -l 'save()'Repository: koder95/eMetrykant
Length of output: 118
Null DataTarget will cause NPE when save() is invoked.
The CabinetWorker.save() method (lines 67–74 in CabinetWorker.java) directly calls dataTarget.setBaptism(), setConfirmation(), setMarriage(), and setDecease() without null checks. Passing null as the target parameter to SimpleCabinetAnalyzer in ApplicationContext (line 40) will trigger an NPE if save() is ever called.
Either provide a no-op DataTarget implementation, add a null check in the save() method, or enforce non-null validation in the constructor.
🤖 Prompt for AI Agents
In `@src/main/java/pl/koder95/eme/bootstrap/ApplicationContext.java` around lines
33 - 42, The ApplicationContext currently passes null for the DataTarget to
SimpleCabinetAnalyzer which leads to an NPE when CabinetWorker.save() calls
dataTarget.setBaptism()/setConfirmation()/setMarriage()/setDecease(); fix this
by providing a no-op DataTarget implementation and use it in the
ApplicationContext constructor instead of null (or alternatively add a
null-check in CabinetWorker.save or validate non-null in SimpleCabinetAnalyzer's
constructor), e.g. implement a NoOpDataTarget that safely no-ops all setter
methods and inject that into new SimpleCabinetAnalyzer(...) so
CabinetWorker.save() can safely call the methods.
| private Book parseBook(Node bookNode) { | ||
| if (bookNode == null || !bookNode.getNodeName().equalsIgnoreCase("book")) { | ||
| return null; | ||
| } | ||
| if (!bookNode.hasAttributes()) { | ||
| return new Book(""); | ||
| } | ||
| String bookName = Optional | ||
| .ofNullable(bookNode.getAttributes().getNamedItem("name")) | ||
| .map(Node::getTextContent) | ||
| .orElseThrow(() -> new IllegalArgumentException("Brak atrybutu 'name' w węźle <book>.")); | ||
| Book book = new Book(bookName); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the IndexLoader.java file and understand the full context
find . -name "IndexLoader.java" -type fRepository: koder95/eMetrykant
Length of output: 113
🏁 Script executed:
# Check the full IndexLoader.java file to see parseBook and loadBooks implementations
cat -n src/main/java/pl/koder95/eme/dfs/IndexLoader.java | head -100Repository: koder95/eMetrykant
Length of output: 2571
🏁 Script executed:
# Search for callers of loadBooks and parseBook to understand error handling
rg -n --type=java -C5 'loadBooks\(|parseBook\('Repository: koder95/eMetrykant
Length of output: 4539
Gracefully skip <book> nodes without a name attribute.
The current code throws IllegalArgumentException when the name attribute is missing (line 49–52). This exception propagates uncaught from loadBooks() and aborts the entire index-loading process, since the caller in IndexList.java (line 116) only catches IOException. If indices.xml is user-edited or partially corrupted, the loader fails completely rather than skipping the malformed book.
For consistency with the existing null checks (line 34–35), consider returning null for books with missing or blank name attributes, optionally logging the issue:
Suggested fix
- String bookName = Optional
- .ofNullable(bookNode.getAttributes().getNamedItem("name"))
- .map(Node::getTextContent)
- .orElseThrow(() -> new IllegalArgumentException("Brak atrybutu 'name' w węźle <book>."));
+ Node nameAttr = bookNode.getAttributes().getNamedItem("name");
+ if (nameAttr == null || nameAttr.getTextContent().isBlank()) {
+ return null;
+ }
+ String bookName = nameAttr.getTextContent();🤖 Prompt for AI Agents
In `@src/main/java/pl/koder95/eme/dfs/IndexLoader.java` around lines 42 - 53,
parseBook currently throws IllegalArgumentException when a <book> node lacks a
name attribute, which aborts loading; change parseBook in IndexLoader so it
returns null instead of throwing when the name attribute is missing or blank
(use Optional.orElse(null) and StringUtils/trim check) and optionally log a
warning via the class logger; then update loadBooks (also in IndexLoader) to
skip null returns from parseBook (do not add them to the list) so malformed
<book> nodes are gracefully ignored at runtime (this preserves behavior expected
by IndexList which only catches IOException).
1 participant
Codex generated this pull request, but encountered an unexpected error after generation. This is a placeholder PR message.
Codex Task
Summary by CodeRabbit
New Features
Refactor