You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@zeppelin.apache.org by "kimyenac (via GitHub)" <gi...@apache.org> on 2026/07/21 06:27:59 UTC
[PR] [ZEPPELIN-6444] Add type guard for imported note JSON to remove @ts-ignore [zeppelin]
kimyenac opened a new pull request, #5333:
URL: https://github.com/apache/zeppelin/pull/5333
### What is this PR for?
`NoteImportComponent` parsed unknown JSON and used three `@ts-ignore` comments to access `result.paragraphs` and `result.name`.
This PR adds a small `isImportNote()` type guard that narrows the parsed value to `ImportNote['note']`, so the paragraph check, the note-name override, and the `messageService.importNote()` call are all type-safe without suppressing the compiler.
### What type of PR is it?
Improvement
### Todos
* [x] Remove the `@ts-ignore` comments in `NoteImportComponent`
* [x] Keep the existing error behavior for invalid / non-note JSON
* [x] Preserve the imported note name override behavior
### What is the Jira issue?
[ZEPPELIN-6444](https://issues.apache.org/jira/browse/ZEPPELIN-6444)
### How should this be tested?
* `cd zeppelin-web-angular && npm run lint` passes.
* Import a valid note JSON (with a non-empty `paragraphs` array) — the note imports; leaving the name field empty keeps the note's original name, filling it overrides the name.
* Import a valid-but-not-a-note JSON (no `paragraphs`) — still shows "Invalid JSON".
* Import a malformed JSON payload — still shows "JSON parse exception".
### Questions:
* Does the license files need to be updated? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6444] Add type guard for imported note JSON to remove @ts-ignore [zeppelin]
Posted by "kimyenac (via GitHub)" <gi...@apache.org>.
kimyenac commented on code in PR #5333:
URL: https://github.com/apache/zeppelin/pull/5333#discussion_r3628159919
##########
zeppelin-web-angular/src/app/share/note-import/note-import.component.ts:
##########
@@ -20,6 +20,14 @@ import { NzUploadFile } from 'ng-zorro-antd/upload';
import { MessageListener, MessageListenersManager } from '@zeppelin/core';
import { ImportNote, MessageReceiveDataTypeMap, OP } from '@zeppelin/sdk';
+const isImportNote = (value: unknown): value is ImportNote['note'] => {
+ if (typeof value !== 'object' || value === null) {
+ return false;
+ }
+ const { paragraphs } = value as { paragraphs?: unknown };
+ return Array.isArray(paragraphs) && paragraphs.length > 0;
+};
Review Comment:
Good call! Reused `isRecord` from `@zeppelin/utility` as suggested. Pushed in `6a952472f`.
Confirmed it's already the repo convention (`workspace.component.ts`, `ng-z.service.ts` use it too), and as you noted it drops the duplication, additionally rules out arrays, and narrows `value` so the cast on `paragraphs` is gone. Thanks! 🙏
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6444] Add type guard for imported note JSON to remove @ts-ignore [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on code in PR #5333:
URL: https://github.com/apache/zeppelin/pull/5333#discussion_r3622577189
##########
zeppelin-web-angular/src/app/share/note-import/note-import.component.ts:
##########
@@ -20,6 +20,14 @@ import { NzUploadFile } from 'ng-zorro-antd/upload';
import { MessageListener, MessageListenersManager } from '@zeppelin/core';
import { ImportNote, MessageReceiveDataTypeMap, OP } from '@zeppelin/sdk';
+const isImportNote = (value: unknown): value is ImportNote['note'] => {
+ if (typeof value !== 'object' || value === null) {
+ return false;
+ }
+ const { paragraphs } = value as { paragraphs?: unknown };
+ return Array.isArray(paragraphs) && paragraphs.length > 0;
+};
Review Comment:
Nice cleanup — the `@ts-ignores` are gone the right way, and the `value === null` check also quietly fixes the old `FileReader.result === null` -> `TypeError` crash 👍
One thing: this object check (`typeof === 'object' && !== null`) is already provided by `isRecord` in `@zeppelin/utility`, so reusing it fits the repo convention here. It drops the duplication, also rules out arrays, and narrows value so the cast isn't needed:
> import { isRecord } from '@zeppelin/utility';
```suggestion
const isImportNote = (value: unknown): value is ImportNote['note'] =>
isRecord(value) && Array.isArray(value.paragraphs) && value.paragraphs.length > 0;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6444] Add type guard for imported note JSON to remove @ts-ignore [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee merged PR #5333:
URL: https://github.com/apache/zeppelin/pull/5333
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6444] Add type guard for imported note JSON to remove @ts-ignore [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on PR #5333:
URL: https://github.com/apache/zeppelin/pull/5333#issuecomment-5059646706
Merged into master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org