DanielLeens commented on PR #10973: URL: https://github.com/apache/seatunnel/pull/10973#issuecomment-5569423624
Thanks for the close read, @SEZ9. On the "cut off" concern: the review isn't actually truncated there — `+ * Copyright (c) 2015-present Ionic (http://ionic.io/)` followed by `+ */` is simply the last line of that attribution header itself (it's a 6-line comment block), and the review continues right after the code fence into the Issue 3 (registry) and Issue 4 (`--omit=dev`) writeups. I re-pulled the raw review body via the API to confirm nothing is missing on the stored side, so this looks like a rendering artifact rather than a truncated post. On your five confirmation asks: 1. **Registry** — confirmed, this is Issue 3 in my last review: I grepped the regenerated `package-lock.json` directly and `registry.npmmirror.com` has zero occurrences (down from 802), `registry.npmjs.org` appears 802 times. Every entry now resolves to the official registry. 2. **`--omit=dev` claim** — confirmed, this is Issue 4: I re-read the current PR description via `gh pr view --json body` and it now correctly states that postcss/autoprefixer/tailwindcss/sass are devDependencies required at `vite build` time, so `--omit=dev` would break the build rather than optimize it; the old claim is gone. 3. **Vendored SVG** — confirmed path-data only. I fetched `src/components/icons/index.ts` at the current head directly: it defines exactly 4 icon components (`DesktopOutline`, `ListOutline`, `PeopleOutline`, `PersonOutline`), and the only tags present are `<path>`, `<rect>`, and `<circle>` with pure geometry/style attributes (`d`, `fill`, `stroke*`) — no `<script>`, no `on*` event-handler attributes, no `href`/`xlink:href`. Nothing executable is vendored. 4. **Native watcher / `--ignore-scripts`** — not verified on my side; I haven't exercised the install with `--ignore-scripts`, so I can't confirm either way whether `@parcel/watcher`'s native install script is a problem on locked-down CI. Given this is exactly the concern behind my own Issue 5 (Low, non-blocking), I'd rather the author test this explicitly than have me guess — worth doing before merge if this repo's CI runners restrict install scripts. 5. **Leftover protobuf lock entry** — still an open gap, not yet a resolved item. I just checked the live PR description and it doesn't currently mention that the `@bufbuild/protobuf`/`sass-embedded` lock entries are now optional/peer after the removal (this is my Issue 6, Low/non-blocking). Agree a short note would help future lockfile-diff readers; I'll ask the author to add it. So: 1 and 2 are fully confirmed fixed as you needed for your own sign-off. 3 is now confirmed safe. 4 and 5 remain open (untested and undocumented, respectively) but both are the same Low-severity, non-blocking items I already carried forward, not new blockers. One factual update since my approval: the required `Build` check, which was still `in_progress` when I reviewed, has since completed and is now `FAILURE` on this same head (`2f9f73416937`). That will need the author's attention before this can actually merge, independent of the source-level review being clear. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
