Conversation
KDS-648 (Empty State)
senemdilli
requested review from
Copilot and
knime-ghub-bot
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
Implements KDS-648 by introducing KDS-based empty state UIs for the Python workspace and view preview, along with dependency updates to support the new components.
Changes:
- Add
KdsEmptyStateto workspace and preview components and adjust conditional rendering accordingly. - Disable KDS legacy mode in the app bootstrap.
- Bump
@knime/kds-componentsand update build/test tooling versions inpackage.json.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| org.knime.python3.scripting.nodes/js-src/src/main.ts | Disables KDS legacy mode globally to use updated KDS behavior/components. |
| org.knime.python3.scripting.nodes/js-src/src/components/PythonWorkspace.vue | Adds KDS empty state when the workspace has no temporary values. |
| org.knime.python3.scripting.nodes/js-src/src/components/PythonViewPreview.vue | Replaces legacy placeholder UI with KdsEmptyState and adds run-action buttons. |
| org.knime.python3.scripting.nodes/js-src/package.json | Updates KDS dependency and upgrades dev tooling versions (vite/vitest/cyclonedx). |
Files not reviewed (1)
- org.knime.python3.scripting.nodes/js-src/package-lock.json: Language not supported
| // const { currentMode } = useKdsDarkMode(); | ||
| // currentMode.value = "dark" | ||
| useKdsLegacyMode(true); | ||
| useKdsLegacyMode(false); |
There was a problem hiding this comment.
The comment says legacy mode can be disabled for development, but the code now disables legacy mode unconditionally. Please update the comment to reflect the new default behavior, or make the legacy-mode toggle explicitly environment-controlled (e.g., dev-only) if that was the intent.
| useKdsLegacyMode(false); | |
| if (import.meta.env.DEV) { | |
| useKdsLegacyMode(false); | |
| } |
| <div v-if="pythonPreviewStatus.hasValidView" class="iframe-container"> | ||
| <iframe | ||
| ref="iframe" | ||
| title="Preview" | ||
| sandbox="allow-scripts" | ||
| :src="IFRAME_SOURCE" | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
Switching from v-show to v-if will unmount/remount the iframe whenever hasValidView toggles, which can force a full reload and lose any internal iframe state. If you want to preserve the iframe instance, keep it mounted (e.g., revert to v-show for the iframe container, or otherwise avoid unmounting it).
| }, | ||
| "devDependencies": { | ||
| "@cyclonedx/cyclonedx-npm": "1.19.3", | ||
| "@cyclonedx/cyclonedx-npm": "^4.1.2", |
There was a problem hiding this comment.
Introducing caret (^) ranges for core build/test tooling can reduce build reproducibility over time (new minors can be picked up when the lockfile is regenerated). If this repo aims for deterministic builds, prefer pinning exact versions for these tools (consistent with the rest of the file) and relying on the lockfile for installs.
| "@types/node": "22.16.0", | ||
| "@vitejs/plugin-vue": "5.2.4", | ||
| "@vitest/coverage-v8": "2.1.9", | ||
| "@vitest/coverage-v8": "^4.0.18", |
There was a problem hiding this comment.
Introducing caret (^) ranges for core build/test tooling can reduce build reproducibility over time (new minors can be picked up when the lockfile is regenerated). If this repo aims for deterministic builds, prefer pinning exact versions for these tools (consistent with the rest of the file) and relying on the lockfile for installs.
| "@vitest/coverage-v8": "^4.0.18", | |
| "@vitest/coverage-v8": "4.0.18", |
| "vite-plugin-monaco-editor": "1.1.0", | ||
| "vite-svg-loader": "5.1.0", | ||
| "vitest": "2.1.9", | ||
| "vitest": "^4.0.18", |
There was a problem hiding this comment.
Introducing caret (^) ranges for core build/test tooling can reduce build reproducibility over time (new minors can be picked up when the lockfile is regenerated). If this repo aims for deterministic builds, prefer pinning exact versions for these tools (consistent with the rest of the file) and relying on the lockfile for installs.
| "vitest": "^4.0.18", | |
| "vitest": "4.0.18", |
|
|
||
| <template> | ||
| <div ref="resizeContainer" class="container"> | ||
| <div v-if="!isEmpty" ref="resizeContainer" class="container"> |
There was a problem hiding this comment.
Using v-if/v-else here will unmount/remount the entire workspace DOM when it transitions between empty/non-empty, which can reset UI state (e.g., scroll position, measured widths) and complicate resize-observer behavior. Consider keeping the outer container mounted and toggling visibility/content inside it (e.g., render the empty state overlay within the same container or use v-show) if preserving state is desired.
| <div v-else class="container-empty-state"> | ||
| <KdsEmptyState | ||
| headline="No temporary values yet" | ||
| description="Run the Python script to display temporary values generated during execution." | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
Using v-if/v-else here will unmount/remount the entire workspace DOM when it transitions between empty/non-empty, which can reset UI state (e.g., scroll position, measured widths) and complicate resize-observer behavior. Consider keeping the outer container mounted and toggling visibility/content inside it (e.g., render the empty state overlay within the same container or use v-show) if preserving state is desired.
1 participant
KDS-648 (Empty State)