chore: fix saving inputs in preferences #13606
Conversation
📝 WalkthroughWalkthroughReplaced two-way binding bind:value with one-way value={value} in StringItem.svelte and moved change propagation to onInput/onChange; also wrapped a single-line if in braces. Added a unit test StringItem.spec.ts that uses userEvent to type "foobar" into the input and asserts onChange is called once per keystroke. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these settings in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The component never resets invalidEntry when onChange resolves, so consider clearing it on success to avoid stale error styling.
- Since you switched from bind:value to a static value attribute, add a test or verify manually that updating the value prop externally still updates the input.
- Add a test to assert that aria-invalid toggles to true when onChange rejects so the error state is fully covered.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The component never resets invalidEntry when onChange resolves, so consider clearing it on success to avoid stale error styling.
- Since you switched from bind:value to a static value attribute, add a test or verify manually that updating the value prop externally still updates the input.
- Add a test to assert that aria-invalid toggles to true when onChange rejects so the error state is fully covered.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/renderer/src/lib/preferences/item-formats/StringItem.spec.ts (1)
81-100: Fix misleading comment and strengthen assertions to verify payload and DOM value
- The inline comment says 9 times but the assertion expects 6. Adjust the comment to avoid confusion.
- Add assertions to verify that onChange receives the correct arguments (id, value) and that the final input value in the DOM is updated as expected. This makes the test more robust and future-proof.
Apply this diff within this block:
- // Ensure it's been called 9 times as "new value" is 6 characters + // Ensure it's been called 6 times as "foobar" is 6 characters await userEvent.type(input, 'foobar'); expect(onChange).toHaveBeenCalledTimes(6); + expect(onChange).toHaveBeenLastCalledWith('record', 'foobar'); + expect(onChange).toHaveBeenNthCalledWith(1, 'record', 'f'); + expect((input as HTMLInputElement).value).toBe('foobar');Optionally, consider adding a separate test to assert that readonly fields do not trigger onChange:
test('typing has no effect and does not call onChange when readonly', async () => { const record: IConfigurationPropertyRecordedSchema = { id: 'record', title: 'record', parentId: 'parent.record', description: 'record-description', type: 'string', readonly: true, }; const onChange = vi.fn(); render(StringItem, { record, value: 'initial', onChange }); const input = screen.getByLabelText('record-description'); await userEvent.type(input, 'ignored'); expect(onChange).not.toHaveBeenCalled(); expect((input as HTMLInputElement).value).toBe('initial'); });packages/renderer/src/lib/preferences/item-formats/StringItem.svelte (1)
25-25: Avoid passing undefined to value; coalesce to an empty stringThe UI Input likely expects a string; passing undefined can lead to inconsistent behavior. Coalesce to ''.
- value={value} + value={value ?? ''}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/renderer/src/lib/preferences/item-formats/StringItem.spec.ts(2 hunks)packages/renderer/src/lib/preferences/item-formats/StringItem.svelte(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/renderer/src/lib/preferences/item-formats/StringItem.spec.ts (1)
packages/api/src/configuration/models.ts (1)
IConfigurationPropertyRecordedSchema(51-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: unit tests / macos-15
- GitHub Check: linter, formatters
- GitHub Check: unit tests / windows-2025
- GitHub Check: typecheck
- GitHub Check: smoke e2e tests (production)
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: smoke e2e tests (development)
- GitHub Check: Windows
- GitHub Check: macOS
- GitHub Check: build website
- GitHub Check: Linux
🔇 Additional comments (3)
packages/renderer/src/lib/preferences/item-formats/StringItem.spec.ts (1)
22-22: LGTM: userEvent import is appropriate for keystroke-level interaction testingImporting userEvent is the right call to simulate per-character typing and input events.
packages/renderer/src/lib/preferences/item-formats/StringItem.svelte (2)
20-29: LGTM: switching from bind:value to value with on:input aligns with Svelte 5 and fixes persistenceMoving to a one-way value prop with on:input-driven updates matches Svelte 5 patterns and addresses the original issue where edits weren’t persisting. Nice.
14-16: Double-check the guard target.value !== value to avoid missing updatesIf the parent updates value synchronously on each keystroke, this guard may occasionally suppress onChange calls (e.g., when value already equals the latest DOM value by the time input fires). This may be fine, but please confirm it doesn’t break any consumers expecting onChange per keystroke.
Would you like me to add a test variant where value is updated in response to onChange to ensure the guard doesn’t skip calls?
| if (record.id && target.value !== value) { | ||
| onChange(record.id, target.value).catch((_: unknown) => (invalidEntry = true)); | ||
| } |
There was a problem hiding this comment.
invalidEntry is never cleared on success; it will remain stuck true after any rejected onChange
Onc 8000 e invalidEntry is set to true on a rejected update, it’s never reset on a subsequent successful change. That keeps aria-invalid stuck and can mislead users/accessibility tooling.
Apply this diff to clear invalidEntry on success (and set it on failure):
- if (record.id && target.value !== value) {
- onChange(record.id, target.value).catch((_: unknown) => (invalidEntry = true));
- }
+ if (record.id && target.value !== value) {
+ onChange(record.id, target.value)
+ .then(() => (invalidEntry = false))
+ .catch(() => (invalidEntry = true));
+ }If you want to avoid out-of-order races, capture the typed value and only set the flag if it still matches:
const next = target.value;
onChange(record.id, next)
.then(() => { if (target.value === next) invalidEntry = false; })
.catch(() => { if (target.value === next) invalidEntry = true; });🤖 Prompt for AI Agents
In packages/renderer/src/lib/preferences/item-formats/StringItem.svelte around
lines 14 to 16, invalidEntry is only set to true on a rejected onChange and
never cleared on success; update the handler to capture the current input value
into a local variable, call onChange(record.id, next), and in the promise
handlers set invalidEntry = false on success and invalidEntry = true on failure
only if the input still equals the captured next value to avoid out-of-order
races.
### What does this PR do? Fixes saving inputs in the preference settings. This is because the value should be `value` rather than `bind:value` (svelte5 convention). Tests have also been added to cover the issue. ### Screenshot / video of UI <!-- If this PR is changing UI, please include screenshots or screencasts showing the difference --> ### What issues does this PR fix or reference? <!-- Include any related issues from Podman Desktop repository (or from another issue tracker). --> Closes podman-desktop#13601 ### How to test this PR? <!-- Please explain steps to verify the functionality, do not forget to provide unit/component tests --> - [X] Tests are covering the bug fix or the new feature Signed-off-by: Charlie Drage <charlie@charliedrage.com>
|
Tested on mac M4 works! |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
chore: fix saving inputs in preferences
What does this PR do?
Fixes saving inputs in the preference settings.
This is because the value should be
valuerather thanbind:value(svelte5 convention).
Tests have also been added to cover the issue.
Screenshot / video of UI
Screen.Recording.2025-08-14.at.11.13.33.AM.mov
What issues does this PR fix or reference?
Closes #13601
How to test this PR?
Signed-off-by: Charlie Drage charlie@charliedrage.com