refactor(icon): fix SVG icons loading straight to memory#56
Open
balazs-szucs wants to merge 8 commits intogrimmory-tools:developfrom
Open
refactor(icon): fix SVG icons loading straight to memory#56balazs-szucs wants to merge 8 commits intogrimmory-tools:developfrom
balazs-szucs wants to merge 8 commits intogrimmory-tools:developfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughBackend removed startup cache preloading and ensured parent directories are created when saving SVGs. Frontend removed automatic post-login icon preloading, introduced a paginated icon-name API call, and refactored the icon-picker to load SVG icons on-demand with concurrent per-icon fetches and error handling. Multiple component subscriptions were bound to teardown. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant IconPicker as Icon Picker Component
participant IconService as Frontend IconService
participant Backend as Backend API
User->>IconPicker: Open icon picker (activate SVG tab)
IconPicker->>IconService: getIconNames(page, size)
IconService->>Backend: GET /api/icons?page=...&size=...
Backend-->>IconService: ["iconA","iconB",...]
IconService-->>IconPicker: icon name list
alt names empty
IconPicker->>IconPicker: clear svgIcons, stop loading
else names present
IconPicker->>IconService: getSvgIconContent(name) [concurrent]
IconService->>Backend: GET /api/icons/{name}/content
Backend-->>IconService: SVG content
IconService-->>IconPicker: sanitized SVG
IconPicker->>IconPicker: aggregate results (toArray), update svgIcons
end
IconPicker-->>User: display icons
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
booklore-api/src/main/java/org/booklore/service/IconService.java (1)
52-61:⚠️ Potential issue | 🔴 CriticalMake icon creation atomic to prevent duplicate-write races.
The
saveSvgIconmethod checks file existence on line 52, then writes withCREATE+TRUNCATE_EXISTINGon lines 59-61. This is a TOCTOU race condition: two concurrent requests can both pass the existence check and one can overwrite the other's file.Use atomic creation (
CREATE_NEW) instead, which atomically creates the file or throwsFileAlreadyExistsExceptionif it already exists. This eliminates the race window.Proposed fix
+import java.nio.file.FileAlreadyExistsException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.StandardOpenOption; @@ - if (Files.exists(filePath)) { - log.warn("SVG icon already exists: {}", filename); - throw ApiError.ICON_ALREADY_EXISTS.createException(request.getSvgName()); - } - try { Files.createDirectories(filePath.getParent()); Files.writeString(filePath, request.getSvgData(), - StandardOpenOption.CREATE, - StandardOpenOption.TRUNCATE_EXISTING); + StandardOpenOption.CREATE_NEW); updateCache(filename, request.getSvgData()); log.info("SVG icon saved successfully: {}", filename); + } catch (FileAlreadyExistsException e) { + log.warn("SVG icon already exists: {}", filename); + throw ApiError.ICON_ALREADY_EXISTS.createException(request.getSvgName()); } catch (IOException e) { log.error("Failed to save SVG icon: {}", e.getMessage(), e); throw ApiError.FILE_READ_ERROR.createException("Failed to save SVG icon: " + e.getMessage()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@booklore-api/src/main/java/org/booklore/service/IconService.java` around lines 52 - 61, The saveSvgIcon method currently does a TOCTOU check with Files.exists(filePath) then writes using CREATE + TRUNCATE_EXISTING; change it to attempt an atomic creation using StandardOpenOption.CREATE_NEW (and remove TRUNCATE_EXISTING) when calling Files.writeString(filePath, request.getSvgData(), StandardOpenOption.CREATE_NEW) after ensuring parent directories exist, and catch FileAlreadyExistsException to throw ApiError.ICON_ALREADY_EXISTS.createException(request.getSvgName()) so concurrent requests cannot overwrite each other; keep other IO exceptions propagated/handled as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@booklore-ui/src/app/shared/components/icon-picker/icon-picker-component.ts`:
- Around line 145-150: The current implementation in loadSvgIcons builds
contentRequests via names.map and passes them to forkJoin, causing up to size
(default 1000) parallel HTTP requests; replace this with a stream-based approach
using from(names) piped through mergeMap to call
iconService.getSvgIconContent(name) only for names that are not cached (use
iconCache.getCachedSanitized(name) to skip), and set mergeMap's concurrency
parameter to a reasonable limit (e.g., 5–20) so requests run bounded
concurrently; collect results (e.g., toArray or accumulating side effects) and
subscribe, preserving the same post-load behavior previously in the forkJoin
subscription.
---
Outside diff comments:
In `@booklore-api/src/main/java/org/booklore/service/IconService.java`:
- Around line 52-61: The saveSvgIcon method currently does a TOCTOU check with
Files.exists(filePath) then writes using CREATE + TRUNCATE_EXISTING; change it
to attempt an atomic creation using StandardOpenOption.CREATE_NEW (and remove
TRUNCATE_EXISTING) when calling Files.writeString(filePath,
request.getSvgData(), StandardOpenOption.CREATE_NEW) after ensuring parent
directories exist, and catch FileAlreadyExistsException to throw
ApiError.ICON_ALREADY_EXISTS.createException(request.getSvgName()) so concurrent
requests cannot overwrite each other; keep other IO exceptions
propagated/handled as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e98bacb-b3c9-44b6-adb3-81f6f7f58c85
📒 Files selected for processing (4)
booklore-api/src/main/java/org/booklore/service/IconService.javabooklore-ui/src/app/core/services/post-login-initializer.service.tsbooklore-ui/src/app/shared/components/icon-picker/icon-picker-component.tsbooklore-ui/src/app/shared/services/icon.service.ts
booklore-ui/src/app/shared/components/icon-picker/icon-picker-component.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@booklore-ui/src/app/shared/components/icon-picker/icon-picker-component.ts`:
- Around line 146-157: The current subscription delays setting
svgIcons/isLoadingSvgIcons until every SVG body is fetched; instead immediately
assign this.svgIcons = names and this.isLoadingSvgIcons = false so the SVG tab
can render names right away, and remove the toArray()-based blocking pipeline;
change the from(names).pipe(...) logic to either kick off non-blocking
background warming (subscribe without awaiting toArray) or move
getSvgIconContent calls to a lazy loader used by the renderer/selection code
(e.g., a method like loadSvgFor(name) that checks
iconCache.getCachedSanitized(name) and calls
iconService.getSvgIconContent(name).pipe(catchError(()=>of(null))) when an item
becomes visible or selected), ensuring background fetches don’t prevent
publishing svgIcons.
- Around line 83-87: Replace the current "load if svgIcons.length === 0" logic
with explicit load-state flags: add booleans isLoadingSvgIcons and
isSvgIconsLoaded, set isLoadingSvgIcons=true at start of loadSvgIcons() and
false in finally, and set isSvgIconsLoaded=true when the fetch completes (even
if the returned array is empty); then update the activeTabIndex setter and any
init logic (e.g., ngOnInit / tab-activation code) to call loadSvgIcons() only
when value === '1' && !isSvgIconsLoaded && !isLoadingSvgIcons to prevent
repeated fetches and duplicate loads when the tab is already active during
initialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e8edebf-69c8-4f60-b834-b36de042b14a
📒 Files selected for processing (1)
booklore-ui/src/app/shared/components/icon-picker/icon-picker-component.ts
booklore-ui/src/app/shared/components/icon-picker/icon-picker-component.ts
Show resolved
Hide resolved
booklore-ui/src/app/shared/components/icon-picker/icon-picker-component.ts
Show resolved
Hide resolved
imajes
previously approved these changes
Mar 20, 2026
imajes
dismissed
their stale review
removing review to make sure it passes tests against develop etc.
Member
|
@balazs-szucs can you rebase this against develop please :) |
## [2.3.0](grimmory-tools/grimmory@v2.2.6...v2.3.0) (2026-03-21) ### Features * **release:** document develop-based stable release previews ([930e526](grimmory-tools@930e526)) ### Bug Fixes * **api:** fix potential memory leaks in file processing ([031e8ae](grimmory-tools@031e8ae)) * **ci:** correct artifact download action pin ([37ca101](grimmory-tools@37ca101)) * **ci:** publish PR test results from workflow_run ([11a76bf](grimmory-tools@11a76bf)) * **ci:** repair release preview and test result publishing ([afa5b81](grimmory-tools@afa5b81)) * drop telemetry from app ([grimmory-tools#52](grimmory-tools#52)) ([4d82cb7](grimmory-tools@4d82cb7)) * **ui:** repair frontend compile after rebrand ([fea1ec6](grimmory-tools@fea1ec6)) ### Refactors * **build:** rename frontend dist output to grimmory ([ecf388f](grimmory-tools@ecf388f)) * **i18n:** rename booklore translation keys to grimmory ([eb94afa](grimmory-tools@eb94afa)) * **metadata:** move default parser from Amazon to Goodreads ([e252122](grimmory-tools@e252122)) * pull kepubify & ffprobe during build ([grimmory-tools#50](grimmory-tools#50)) ([1c15629](grimmory-tools@1c15629)) * **ui:** rebrand frontend surfaces to grimmory ([d786dd8](grimmory-tools@d786dd8)) ### Chores * **api:** remove the custom startup banner ([98c9b1a](grimmory-tools@98c9b1a)) * **deps:** bump flatted from 3.4.1 to 3.4.2 in /booklore-ui ([grimmory-tools#73](grimmory-tools#73)) ([c4bd0c7](grimmory-tools@c4bd0c7)) * **funding:** point support links at opencollective ([55c0ac0](grimmory-tools@55c0ac0)) * **release:** 2.2.7 [skip ci] ([0b5e24c](grimmory-tools@0b5e24c)) * remove old verbose PR template, replace with temporary more low-key one. ([grimmory-tools#84](grimmory-tools#84)) ([b868526](grimmory-tools@b868526)) * **ui:** drop financial support dialog ([grimmory-tools#21](grimmory-tools#21)) ([62be6b1](grimmory-tools@62be6b1)) ### Documentation * updated supported file formats in README.md ([grimmory-tools#68](grimmory-tools#68)) ([f912e80](grimmory-tools@f912e80)) ### Style * **i18n:** normalize translation json formatting ([grimmory-tools#89](grimmory-tools#89)) ([857290d](grimmory-tools@857290d)) * **ui:** simplify the topbar logo branding ([0416d48](grimmory-tools@0416d48))
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@booklore-ui/src/app/features/book/components/book-browser/book-browser.component.ts`:
- Line 299: The subscription to coverScalePreferenceService.scaleChange$
currently discards values
(this.coverScalePreferenceService.scaleChange$.pipe(debounceTime(1000),
takeUntil(this.destroy$)).subscribe();) and should be removed or given an
explicit handler and comment; either delete this line entirely if the component
has no side-effects on scale changes (the service already persists via
saveScalePreference()), or replace subscribe() with a documented callback that
performs the intended component-side work (e.g., update local UI state or call a
refresh method) and keep the debounceTime(1000) and takeUntil(this.destroy$) for
cleanup; refer to coverScalePreferenceService, scaleChange$,
saveScalePreference(), debounceTime and destroy$ when locating the code to
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: adc9cf4d-0136-43d1-9bda-7083d6252c3e
📒 Files selected for processing (1)
booklore-ui/src/app/features/book/components/book-browser/book-browser.component.ts
booklore-ui/src/app/features/book/components/book-browser/book-browser.component.ts
Show resolved
Hide resolved
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
3 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.
📝 Description
Backend
@PostConstructinitialization and icon cache loading fromIconService, eliminating eager loading of all SVG icons at startup.Frontend
Linked Issue: Fixes #
🏷️ Type of Change
🔧 Changes
🧪 Testing (MANDATORY)
Manual testing steps you performed:
Regression testing:
Edge cases covered:
Test output:
Backend test output (
./gradlew test)Frontend test output (
ng test)📸 Screen Recording / Screenshots (MANDATORY)
✅ Pre-Submission Checklist
develop(merge conflicts resolved)🤖 AI-Assisted Contributions
TODOs, or unused scaffolding left behind by AI💬 Additional Context (optional)
Summary by CodeRabbit
Performance Improvements
Bug Fixes