fix: improve code quality with type annotations and error handling#14570
Open
N3XT3R1337 wants to merge 1 commit intovuejs:mainfrom
Open
fix: improve code quality with type annotations and error handling#14570N3XT3R1337 wants to merge 1 commit intovuejs:mainfrom
N3XT3R1337 wants to merge 1 commit intovuejs:mainfrom
Conversation
📝 WalkthroughWalkthroughThe pull request adds defensive parsing to handle falsy intermediate values in watch path traversal and malformed style segments without colons in style patching. Each fix includes corresponding test cases to verify the corrected behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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)
📝 Coding Plan
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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@packages/runtime-core/__tests__/apiWatch.spec.ts`:
- Around line 1449-1474: The test currently watches 'a.b' so the falsy value 0
is the terminal value; change the watched path to include an extra segment (e.g.
'a.b.c') so the falsy 0 is an intermediate segment: update the definition in the
test (Comp.data) to keep a.b = 0 as the intermediate falsy, change the
this.$watch call in created to this.$watch('a.b.c', spy), and in mounted assign
this.a.b = { c: 1 } (or set this.a.b.c = 1 after replacing b with an object) so
the watcher exercises traversal over a falsy intermediate value and the
assertions expect the new value 1 and previous undefined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36a289ef-bf43-4323-ac29-1f99debf10c0
📒 Files selected for processing (4)
packages/runtime-core/__tests__/apiWatch.spec.tspackages/runtime-core/src/apiWatch.tspackages/runtime-dom/__tests__/patchStyle.spec.tspackages/runtime-dom/src/modules/style.ts
Comment on lines
+1449
to
+1474
| test('watching keypath should not stop on falsy intermediate values', async () => { | ||
| const spy = vi.fn() | ||
| const Comp = defineComponent({ | ||
| render() {}, | ||
| data() { | ||
| return { | ||
| a: { | ||
| b: 0 as any, | ||
| }, | ||
| } | ||
| }, | ||
| created(this: any) { | ||
| this.$watch('a.b', spy) | ||
| }, | ||
| mounted(this: any) { | ||
| this.a.b = 1 | ||
| }, | ||
| }) | ||
|
|
||
| const root = nodeOps.createElement('div') | ||
| createApp(Comp).mount(root) | ||
|
|
||
| await nextTick() | ||
| expect(spy).toHaveBeenCalledTimes(1) | ||
| expect(spy).toHaveBeenCalledWith(1, 0, expect.anything()) | ||
| }) |
There was a problem hiding this comment.
Test case does not exercise a falsy intermediate segment.
On Line 1449, a.b makes 0 the terminal value. This can pass even without the cur != null fix, so the regression target is under-tested.
✅ Suggested test addition to cover a true intermediate-falsy traversal
+ test('watching keypath should continue through falsy intermediate values', async () => {
+ const spy = vi.fn()
+ const Comp = defineComponent({
+ render() {},
+ data() {
+ return {
+ a: {
+ b: '' as any,
+ },
+ }
+ },
+ created(this: any) {
+ this.$watch('a.b.value', spy)
+ },
+ mounted(this: any) {
+ this.a.b = { value: 1 }
+ },
+ })
+
+ const root = nodeOps.createElement('div')
+ createApp(Comp).mount(root)
+
+ await nextTick()
+ expect(spy).toHaveBeenCalledTimes(1)
+ expect(spy).toHaveBeenCalledWith(1, undefined, expect.anything())
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test('watching keypath should not stop on falsy intermediate values', async () => { | |
| const spy = vi.fn() | |
| const Comp = defineComponent({ | |
| render() {}, | |
| data() { | |
| return { | |
| a: { | |
| b: 0 as any, | |
| }, | |
| } | |
| }, | |
| created(this: any) { | |
| this.$watch('a.b', spy) | |
| }, | |
| mounted(this: any) { | |
| this.a.b = 1 | |
| }, | |
| }) | |
| const root = nodeOps.createElement('div') | |
| createApp(Comp).mount(root) | |
| await nextTick() | |
| expect(spy).toHaveBeenCalledTimes(1) | |
| expect(spy).toHaveBeenCalledWith(1, 0, expect.anything()) | |
| }) | |
| test('watching keypath should not stop on falsy intermediate values', async () => { | |
| const spy = vi.fn() | |
| const Comp = defineComponent({ | |
| render() {}, | |
| data() { | |
| return { | |
| a: { | |
| b: 0 as any, | |
| }, | |
| } | |
| }, | |
| created(this: any) { | |
| this.$watch('a.b', spy) | |
| }, | |
| mounted(this: any) { | |
| this.a.b = 1 | |
| }, | |
| }) | |
| const root = nodeOps.createElement('div') | |
| createApp(Comp).mount(root) | |
| await nextTick() | |
| expect(spy).toHaveBeenCalledTimes(1) | |
| expect(spy).toHaveBeenCalledWith(1, 0, expect.anything()) | |
| }) | |
| test('watching keypath should continue through falsy intermediate values', async () => { | |
| const spy = vi.fn() | |
| const Comp = defineComponent({ | |
| render() {}, | |
| data() { | |
| return { | |
| a: { | |
| b: '' as any, | |
| }, | |
| } | |
| }, | |
| created(this: any) { | |
| this.$watch('a.b.value', spy) | |
| }, | |
| mounted(this: any) { | |
| this.a.b = { value: 1 } | |
| }, | |
| }) | |
| const root = nodeOps.createElement('div') | |
| createApp(Comp).mount(root) | |
| await nextTick() | |
| expect(spy).toHaveBeenCalledTimes(1) | |
| expect(spy).toHaveBeenCalledWith(1, undefined, expect.anything()) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/runtime-core/__tests__/apiWatch.spec.ts` around lines 1449 - 1474,
The test currently watches 'a.b' so the falsy value 0 is the terminal value;
change the watched path to include an extra segment (e.g. 'a.b.c') so the falsy
0 is an intermediate segment: update the definition in the test (Comp.data) to
keep a.b = 0 as the intermediate falsy, change the this.$watch call in created
to this.$watch('a.b.c', spy), and in mounted assign this.a.b = { c: 1 } (or set
this.a.b.c = 1 after replacing b with an object) so the watcher exercises
traversal over a falsy intermediate value and the assertions expect the new
value 1 and previous undefined.
Member
|
Thanks for your 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.
Code quality improvements found during code review. Added type annotations, improved error handling, fixed edge cases.
Summary by CodeRabbit
Bug Fixes