API key persistence setting should be per fetcher - #16616
Conversation
PR Summary by QodoMake API key persistence a per-fetcher preference
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1.
|
| // Clear keys that should not be persisted | ||
| if (!keysToClear.isEmpty()) { | ||
| writeKeyring(keysToClear); | ||
| } |
There was a problem hiding this comment.
2. Trivial comments in storefetcherkeys 📘 Rule violation ⚙ Maintainability
New comments in storeFetcherKeys/clearCustomFetcherKeys restate what the immediately following code already makes obvious. This adds noise without documenting rationale.
Agent Prompt
## Issue description
Comments were added that restate the code ("Clear keys that should not be persisted", "Write keys that should be persisted", "Remove the preference keys") rather than explaining intent/why.
## Issue Context
Compliance requires comments to explain rationale and avoid trivial restatements.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[2468-2475]
- jablib/src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java[2487-2490]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…tting apply only on OK) and add backward compatibility note to changelog
Remove useless comment
|
Your pull request conflicts with the target branch. Please merge |
LoayTarek5
left a comment
There was a problem hiding this comment.
Great work @kerollosy
| @@ -2409,16 +2406,33 @@ private void storeCustomImportFormats(Set<CustomImporter> importers) { | |||
| } | |||
|
|
|||
| private Set<FetcherApiKey> getFetcherKeys(Set<FetcherApiKey> defaults) { | |||
There was a problem hiding this comment.
Could we restore the guard above the persist block? the new else branch at around 2431-2433 line already covers a missing PERSIST key, right?
without it a fresh profile returns an empty set instead so the dialog stops prefiling the bundled key
| for (FetcherApiKey apiKey : defaults.getApiKeys()) { | ||
| names.add(apiKey.getName()); | ||
| uses.add(String.valueOf(apiKey.shouldUse())); | ||
| keys.put(KeyringSlot.customApiKey(apiKey.getName()), apiKey.getKey()); | ||
| persists.add(String.valueOf(apiKey.shouldPersist())); | ||
| if (apiKey.shouldPersist()) { | ||
| keysToPersist.put(KeyringSlot.customApiKey(apiKey.getName()), apiKey.getKey()); | ||
| } else { | ||
| keysToClear.put(KeyringSlot.customApiKey(apiKey.getName()), ""); | ||
| } | ||
| } | ||
|
|
||
| putStringList(FETCHER_CUSTOM_KEY_NAMES, names); | ||
| putStringList(FETCHER_CUSTOM_KEY_USES, uses); | ||
| putStringList(FETCHER_CUSTOM_KEY_PERSIST, persists); | ||
|
|
||
| if (defaults.shouldPersistCustomKeys()) { | ||
| writeKeyring(keys); | ||
| } else { | ||
| clearCustomFetcherKeys(); | ||
| if (!keysToClear.isEmpty()) { | ||
| writeKeyring(keysToClear); | ||
| } | ||
| if (!keysToPersist.isEmpty()) { | ||
| writeKeyring(keysToPersist); | ||
| } |
There was a problem hiding this comment.
writeKeyring already deletes on a blank value, so this collapses to one map in the existing loop
| boolean legacyPersist = Boolean.parseBoolean(rawPersists.getFirst()); | ||
| persists = Collections.nCopies(names.size(), String.valueOf(legacyPersist)); | ||
| // Normalize storage to list format | ||
| putStringList(FETCHER_CUSTOM_KEY_PERSIST, persists); |
There was a problem hiding this comment.
I think it better to drop the write and let the next save normalize, what do you think?
bec it writes before the size check, it is also redundant bec storeFetcherKeys rewrites PERSIST below anyway
| // Remove the preference keys | ||
| remove(FETCHER_CUSTOM_KEY_NAMES); | ||
| remove(FETCHER_CUSTOM_KEY_USES); | ||
| remove(FETCHER_CUSTOM_KEY_PERSIST); |
There was a problem hiding this comment.
I think these three remove() calls can not do anything, since the else clearCustomFetcherKeys branch is gone, so it is dead code
| private final WebSearchTabViewModel viewModel; | ||
| private final FetcherViewModel fetcherViewModel; | ||
| private final BooleanProperty apiKeyValid = new SimpleBooleanProperty(); | ||
| private final BooleanProperty persistLocal; |
There was a problem hiding this comment.
persistLocal may not be needed bec convertResult reads apiKeyField.getText directly.
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
|
||
| class ImporterPreferencesTest { |
There was a problem hiding this comment.
I think the test file need more work, bec no tests added, and main has none for getFetcherKeys or storeFetcherKeys also for the persist resolution (raw stored list + name count flags) makes it testable
Summary
Changed the API key persistence mechanism from a global flag to a per‑fetcher setting. Each fetcher now stores its own “save for future use” preference, which is read and written alongside the API key in the preferences system and the system keyring. This resolves the issue where the last edited fetcher’s persistence choice was applied to all fetchers and ensures that enabling/disabling persistence for one fetcher does not affect others.
jabref-contrib-policy:4.2:reviewed:okSteps to test
Related issues and pull requests
Closes #16609, related to #16519, #16250
AI usage
Claude Code (model claude-opus-5) was used to help navigate through the files
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)