fix(metadata): require Google Books API key - #1795
Merged
imnotjames merged 4 commits intoJul 14, 2026
Merged
Conversation
Contributor
WalkthroughThe Google Books metadata provider now derives its enabled state from both the stored flag and whether a trimmed API key is present. The template disables the toggle until a key exists and marks the key field as required. The spec now covers hydration and save behavior. ChangesGoogle Books API Key Enforcement
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.spec.ts`:
- Around line 46-84: Add a new test case in the metadata-provider-settings
component test file to verify the happy path scenario for Google Books
hydration. The test should use appSettingsSignal.set() with buildSettings()
passing enabled: true and a non-empty apiKey (e.g., 'valid-key'), call
fixture.detectChanges(), and then assert that both component.googleEnabled and
component.googleApiKeyConfigured are true. This test complements the existing
test "does not hydrate Google Books as enabled without an API key" by covering
the positive case where hydration should succeed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4552bf3a-d881-48c3-8993-6024846d3f51
⛔ Files ignored due to path filters (1)
frontend/src/i18n/en/settings-metadata.jsonis excluded by!frontend/src/i18n/**
📒 Files selected for processing (3)
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.htmlfrontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.spec.tsfrontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
frontend/src/**/*.{ts,tsx,html,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation in TypeScript, HTML, and SCSS in frontend code
Files:
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.spec.tsfrontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.htmlfrontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.ts
frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer inject() over constructor injection in frontend code
Files:
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.spec.tsfrontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.ts
frontend/src/**/*.{ts,tsx,html}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/src/**/*.{ts,tsx,html}: Follow frontend/eslint.config.js: component selectors use app-, directive selectors use app, and any is disallowed in frontend code
Put user-facing strings in Transloco files under frontend/src/i18n/
Files:
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.spec.tsfrontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.htmlfrontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.ts
frontend/src/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for tests in frontend code
Files:
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.spec.ts
**/*
⚙️ CodeRabbit configuration file
**/*: This project is being developed using current and future-facing technologies:
- Java 25 with --enable-preview (preview features are INTENTIONAL and encouraged)
- Spring Boot 4 (latest major version, check APIs accordingly)
- Jackson 3 (new package: tools.jackson.* instead of com.fasterxml.jackson.*)
- Hibernate 7.3.x (Jakarta Persistence 3.2, new APIs; avoid deprecated Hibernate 5/6 patterns)
- Angular 21 (signals-based reactivity, no NgModules unless legacy)
Grimmory Internal Tools
- epub4j and pdfium4j are our own internal tools developed by the Grimmory team.
- Always verify behavior and API changes against the upstream repositories:
- If you encounter issues with these libraries, check if a fix exists in the upstream grimmory-tools organization.
Metadata Standards and Compliance
- For all metadata writing and parsing logic, double-check against Dublin Core and ANSI standards to ensure perfect official compliance.
- We strictly follow the widespread and official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
General Java and Spring rules
- ALWAYS prefer modern, idiomatic Java 25 constructs over legacy patterns.
- Preview features (--enable-preview) are enabled and intentional; do NOT flag them as risky unless there is a concrete runtime issue.
- Prefer: records, sealed classes/interfaces, pattern matching (switch expressions, instanceof), structured concurrency (StructuredTaskScope), scoped values, string templates, unnamed patterns/variables.
- Prefer virtual threads (Thread.ofVirtual(), Executors.newVirtualThreadPerTaskExecutor()) over platform threads for I/O-bound work.
- Prefer the new Sequenced Collections API (SequencedCollection, SequencedMap) where applicable.
- Prefer
varfor local variables when the type is obvious from context.- Use stream().toList() instead of stream().collect(Collectors.toList()) for imm...
Files:
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.spec.tsfrontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.htmlfrontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.ts
**/*.spec.ts
⚙️ CodeRabbit configuration file
**/*.spec.ts: Angular 21 test review:
- Flag tests with no expect() calls.
- Flag hardcoded async timeouts; prefer fakeAsync/tick or signal-based testing.
- Flag missing fixture.detectChanges() after state mutations.
Files:
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.spec.ts
frontend/src/**/*.component.ts
📄 CodeRabbit inference engine (AGENTS.md)
Keep Angular code on standalone components. Do not add NgModules in frontend code
Files:
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.ts
**/*.component.ts
⚙️ CodeRabbit configuration file
**/*.component.ts: Angular 21 component review:
- Must be standalone (standalone: true); flag NgModule-based declarations.
- Prefer OnPush change detection strategy.
- Prefer inject() over constructor DI.
- Localization: use Transloco for all UI strings.
- Prefer Angular Signals (signal, computed, effect) for local/reactive state.
- Flag missing trackBy / track expression in list rendering.
- Prefer
@if/@for/@switchover *ngIf / *ngFor / *ngSwitch structural directives.
Files:
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.ts
🧠 Learnings (4)
📚 Learning: 2026-04-05T21:16:01.715Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 385
File: frontend/src/app/app.component.ts:55-56
Timestamp: 2026-04-05T21:16:01.715Z
Learning: When reviewing code in the Grimmory frontend (Angular), prefer modern Angular patterns. Specifically: (1) Prefer `DestroyRef` with `takeUntilDestroyed(destroyRef)` for teardown in Angular v16+ instead of manually tracking `Subscription` arrays and calling `unsubscribe()` in `ngOnDestroy()`. (2) Prefer `inject()` for dependency injection over constructor injection where appropriate. (3) Prefer Angular signals (e.g., `signal`, `computed`) over `BehaviorSubject`/`Observable` for state where signals/computed values fit the use case. Flag older patterns when they can be replaced with these modern equivalents without changing behavior.
Applied to files:
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.spec.tsfrontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.ts
📚 Learning: 2026-04-07T09:28:09.587Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 393
File: frontend/src/app/features/readers/pdf-reader/pdf-reader.component.ts:255-263
Timestamp: 2026-04-07T09:28:09.587Z
Learning: In this Angular frontend (under frontend/src/app/), flag manual resource management/cleanup patterns when there is an Angular v16+ automatic alternative. Examples to prefer: (1) Instead of manually pairing document/window event listeners with stored cleanup functions (e.g., add/removeEventListener with mouseMoveCleanup/documentClickCleanup/keydownCleanup/touchCleanup fields), register teardown via DestroyRef.onDestroy(cleanupFn) (or equivalent Angular v16+ teardown mechanism). (2) Instead of storing Subscriptions in fields and explicitly unsubscribing in ngOnDestroy (e.g., annotationSaveSubscription/annotationCacheSubscription), use takeUntilDestroyed(destroyRef) (piped into the observable) or other Angular v16+ primitives. (3) If teardown is lifecycle-coupled and can be automated via DestroyRef/takeUntilDestroyed/signals (or other Angular v16+ mechanisms), prefer the automated approach over manual ngOnDestroy cleanup. Raise a review finding for the manual pattern and recommend the aut...
Applied to files:
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.spec.tsfrontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.ts
📚 Learning: 2026-04-11T03:55:57.229Z
Learnt from: zachyale
Repo: grimmory-tools/grimmory PR: 439
File: frontend/src/app/features/series-browser/components/series-browser/series-browser.component.ts:178-196
Timestamp: 2026-04-11T03:55:57.229Z
Learning: In this Angular frontend (frontend/src/app/), prefer the team’s reactive i18n “signal/computed” pattern: (1) For individual reactive translated strings, use `translateSignal()` from `jsverse/transloco`. (2) For option/label arrays that must update on language switch, create a single `activeLang` signal with `toSignal(t.langChanges$, { initialValue: t.getActiveLang() })`, then derive the arrays as `computed()` signals that read `activeLang()`. This should avoid manual `langChanges$` subscriptions and any `ngOnDestroy` subscription cleanup; prefer this over subscribing in `ngOnInit` when implementing reactive localization.
Applied to files:
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.spec.tsfrontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.ts
📚 Learning: 2026-05-18T14:54:39.422Z
Learnt from: alexhb1
Repo: grimmory-tools/grimmory PR: 1379
File: frontend/src/assets/styles/tailwind.css:3-4
Timestamp: 2026-05-18T14:54:39.422Z
Learning: In the grimmory-tools/grimmory repository, Biome is not used for linting/formatting (no `biome.json` and no Biome dependency in `package.json`). During code reviews, do not raise Biome-related issues or recommend adding/changing `biome.json`/Biome dependencies for formatting or linting in this project.
Applied to files:
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.spec.tsfrontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.ts
🪛 HTMLHint (1.9.2)
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.html
[warning] 77-77: No matching [ label ] tag found.
(input-requires-label)
🔇 Additional comments (7)
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.ts (3)
121-121: LGTM!
144-153: LGTM!
171-173: LGTM!frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.html (1)
54-84: LGTM!frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.spec.ts (3)
1-44: LGTM!
46-84: LGTM!
86-106: LGTM!
Contributor
|
Can you match the description to the template? Thanks |
Contributor
Author
|
Updated the PR description to match the repository template, including manual testing steps, checklist state, AI disclosure, and clarification that the optional confirmation-checkbox behavior is deferred. I also added the positive Google Books hydration test requested in review; the focused suite now passes 5/5. |
imnotjames
reviewed
Jul 6, 2026
Contributor
Author
|
Thanks for the review. Addressed this in 6dc385d by renaming the label key to googleApiLabel and removing the extra Google API key change handler, so the component now relies on the existing configured-key guard during hydration/save. Focused spec passes locally: pnpm --filter grimmory exec vitest run src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.spec.ts. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.html (1)
73-82: 🎯 Functional Correctness | 🔵 TrivialUpdate documentation to reflect required API key.
The linked repository
grimmory-tools/grimmory-docsdocuments the Google Books API key as optional atdocs/metadata/metadata-settings.md:13, stating it only "increases rate limits." This conflicts with the PR's new requirement that Google Books cannot be enabled without a non-empty key. A follow-up PR should update the documentation to reflect the new mandatory status.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.html` around lines 73 - 82, Update the Google Books API key documentation in the metadata settings guide to state that a non-empty key is required before Google Books can be enabled, replacing the current optional/rate-limit-only description. Keep the documentation aligned with the validation behavior represented by the googleApiKey setting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.html`:
- Around line 73-82: Update the Google Books API key documentation in the
metadata settings guide to state that a non-empty key is required before Google
Books can be enabled, replacing the current optional/rate-limit-only
description. Keep the documentation aligned with the validation behavior
represented by the googleApiKey setting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: e80d3f07-bcdd-4c85-8d44-e9d6731c8f4d
📒 Files selected for processing (1)
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.html
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
grimmory-tools/grimmory-docs(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
frontend/src/**/*.{ts,tsx,html,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indentation in TypeScript, HTML, and SCSS in frontend code
Files:
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.html
frontend/src/**/*.{ts,tsx,html}
📄 CodeRabbit inference engine (AGENTS.md)
frontend/src/**/*.{ts,tsx,html}: Follow frontend/eslint.config.js: component selectors use app-, directive selectors use app, and any is disallowed in frontend code
Put user-facing strings in Transloco files under frontend/src/i18n/
Files:
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.html
**/*
⚙️ CodeRabbit configuration file
**/*: This project is being developed using current and future-facing technologies:
- Java 25 with --enable-preview (preview features are INTENTIONAL and encouraged)
- Spring Boot 4 (latest major version, check APIs accordingly)
- Jackson 3 (new package: tools.jackson.* instead of com.fasterxml.jackson.*)
- Hibernate 7.3.x (Jakarta Persistence 3.2, new APIs; avoid deprecated Hibernate 5/6 patterns)
- Angular 21 (signals-based reactivity, no NgModules unless legacy)
Grimmory Internal Tools
- epub4j and pdfium4j are our own internal tools developed by the Grimmory team.
- Always verify behavior and API changes against the upstream repositories:
- If you encounter issues with these libraries, check if a fix exists in the upstream grimmory-tools organization.
Metadata Standards and Compliance
- For all metadata writing and parsing logic, double-check against Dublin Core and ANSI standards to ensure perfect official compliance.
- We strictly follow the widespread and official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
General Java and Spring rules
- ALWAYS prefer modern, idiomatic Java 25 constructs over legacy patterns.
- Preview features (--enable-preview) are enabled and intentional; do NOT flag them as risky unless there is a concrete runtime issue.
- Prefer: records, sealed classes/interfaces, pattern matching (switch expressions, instanceof), structured concurrency (StructuredTaskScope), scoped values, string templates, unnamed patterns/variables.
- Prefer virtual threads (Thread.ofVirtual(), Executors.newVirtualThreadPerTaskExecutor()) over platform threads for I/O-bound work.
- Prefer the new Sequenced Collections API (SequencedCollection, SequencedMap) where applicable.
- Prefer
varfor local variables when the type is obvious from context.- Use stream().toList() instead of stream().collect(Collectors.toList()) for imm...
Files:
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.html
🧠 Learnings (1)
📚 Learning: 2026-06-30T01:30:43.644Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 1881
File: frontend/src/app/shared/components/icon-picker/icon-picker-component.ts:116-119
Timestamp: 2026-06-30T01:30:43.644Z
Learning: In the Grimmory Angular frontend (Angular 21), do not flag `[(ngModel)]` two-way bindings that are bound directly to a `WritableSignal`. Angular supports two-way binding to writable signals, so usages like `[(ngModel)]="svgSearchText"` should be treated as valid and should not be considered an incorrect overwrite of the signal with a plain value. Only flag if the binding target is not a `WritableSignal`.
Applied to files:
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.html
🔀 Multi-repo context grimmory-tools/grimmory-docs
Linked repositories findings
grimmory-tools/grimmory-docs
docs/metadata/metadata-settings.md:13documents the Google Books API key as optional and states it only “increases rate limits.” This conflicts with the PR’s new requirement that Google Books cannot be enabled without a non-empty key.[::grimmory-tools/grimmory-docs::]
🔇 Additional comments (3)
frontend/src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.html (3)
54-57: LGTM!
158-163: LGTM!
73-82: 🎯 Functional CorrectnessNo change needed for the Google Books API key label.
The description already states that a Google Books API key is required to enable the provider.> Likely an incorrect or invalid review comment.
imnotjames
approved these changes
Jul 11, 2026
imnotjames
left a comment
imnotjames
left a comment
Contributor
There was a problem hiding this comment.
Works well enough for now.
Contributor
|
Thanks for the contribution! |
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.
Description
Require a non-empty Google Books API key before the metadata provider can be enabled. The key input remains available while the provider is disabled, clearing the key turns the provider off, and the save payload defensively prevents persisting
enabled: truewithout a key.Linked Issue
Fixes #1435
Changes
Manual Testing Steps
pnpm exec ng test --watch=false --include=src/app/features/settings/global-preferences/metadata-provider-settings/metadata-provider-settings.component.spec.ts(5/5passed).pnpm run typecheck,pnpm run lint, andpnpm run build; all passed.1624tests passed and one unrelatedbook-dialog-helper.service.spec.tstest timed out; that spec passed3/3when rerun independently.Additional Context (Optional)
The local environment uses Node
22.18.0, while the repository declares Node24+. The optional confirmation-checkbox behavior from the issue was not included; this PR implements the core requirement that an API key is necessary to enable Google Books.AI Disclosure
OpenAI Codex - substantially assisted with codebase investigation, implementation, test drafting, and review follow-up. I reviewed the changes and verified them locally using the commands above.
Checklist
just ui checkandjust api check.Summary by CodeRabbit
New Features
Bug Fixes
Tests