Add privacy note to dataset upload #8354
Conversation
📝 WalkthroughWalkthroughThe changes update the WebKnossos changelog and user interface to highlight that imported datasets are private by default. A new component, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/admin/dataset/dataset_components.tsx (1)
1-1: Remove unused imports.The following imports are not used in the code:
WarningOutlinedfrom@ant-design/iconsPRIMARY_COLORfromoxalis/constantsLinkfromreact-router-domApply this diff to remove the unused imports:
-import { WarningOutlined } from "@ant-design/icons"; import { Card, Form, type FormInstance, Input, Select, Space } from "antd"; import { FormItemWithInfo } from "dashboard/dataset/helper_components"; import TeamSelectionComponent from "dashboard/dataset/team_selection_component"; import features from "features"; import messages from "messages"; -import { PRIMARY_COLOR } from "oxalis/constants"; import type * as React from "react"; -import { Link } from "react-router-dom";Also applies to: 7-7, 9-9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx(2 hunks)frontend/javascripts/admin/dataset/dataset_components.tsx(3 hunks)frontend/javascripts/admin/dataset/dataset_upload_view.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/javascripts/admin/dataset/dataset_add_remote_view.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (4)
frontend/javascripts/admin/dataset/dataset_components.tsx (1)
Line range hint
14-43: LGTM! Clean implementation of the subtitle prop.The CardContainer component has been updated to support an optional subtitle prop with proper TypeScript typing and clean rendering implementation.
frontend/javascripts/admin/dataset/dataset_upload_view.tsx (2)
184-194: LGTM! Well-structured privacy info component.The dataPrivacyInfo component is well-implemented with:
- Clear and concise messaging
- Proper use of Space component for layout
- Semantic link to documentation
723-723: LGTM! Proper integration of privacy info.The privacy note is correctly integrated into the CardContainer component.
CHANGELOG.unreleased.md (1)
18-18: LGTM! Clear and well-formatted changelog entry.The changelog entry is properly formatted and clearly communicates the addition of the privacy note feature.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/javascripts/admin/dataset/dataset_components.tsx (2)
7-7: Remove unused import.The
Linkcomponent fromreact-router-domis imported but not used in this file.-import { Link } from "react-router-dom";
34-41: Consider moving inline styles to a CSS module.While the implementation is correct, consider moving the inline styles to a CSS module for better maintainability and reusability.
+// styles.module.css +.title { + line-height: 10px; +} + +.titleWithSubtitle { + margin-top: 22px; +} + +.titleWithoutSubtitle { + margin-top: 12px; +} + +.subtitle { + font-size: 12px; + margin-top: 0; + color: grey; +} // dataset_components.tsx +import styles from './styles.module.css'; -title={ - <> - <h3 style={{ lineHeight: "10px", marginTop: subtitle != null ? "22px" : "12px" }}> - {title} - </h3> - <span style={{ fontSize: 12, marginTop: 0, color: "grey" }}>{subtitle}</span> - </> -} +title={ + <> + <h3 className={`${styles.title} ${subtitle != null ? styles.titleWithSubtitle : styles.titleWithoutSubtitle}`}> + {title} + </h3> + <span className={styles.subtitle}>{subtitle}</span> + </> +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/admin/dataset/dataset_components.tsx(3 hunks)
🔇 Additional comments (1)
frontend/javascripts/admin/dataset/dataset_components.tsx (1)
16-16: LGTM!The
subtitleprop is well-typed as an optionalJSX.Element, which is appropriate for React elements.Also applies to: 21-21
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/admin/dataset/dataset_upload_view.tsx (1)
184-196: Consider adding ARIA label for better accessibility.The privacy note implementation looks good, with proper security attributes for the external link. Consider adding an aria-label to the Space component to improve accessibility.
- <Space direction="horizontal" size={4}> + <Space direction="horizontal" size={4} aria-label="Privacy Information">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/admin/dataset/dataset_components.tsx(2 hunks)frontend/javascripts/admin/dataset/dataset_upload_view.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.unreleased.md
- frontend/javascripts/admin/dataset/dataset_components.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (2)
frontend/javascripts/admin/dataset/dataset_upload_view.tsx (2)
21-21: LGTM!The Space component import is correctly placed with other antd imports.
725-725: LGTM!The privacy note is correctly integrated into the CardContainer component.
There was a problem hiding this comment.
Very nice 🎉
Thanks a lot for adding this info to the upload / ... views. In only found a minor nitpick.
Moreover, I think it would be nice to have the info also in the Card of the Compose a dataset from existing dataset layers view. So that it is present in all three tabs and not only in two of them. Because now as the info is missing in the third tab, it feels like when I compose the dataset, the default private rule does not apply. But afaik it does. So I think it would be nice to have it added there as well :)
Besides this everything is 🔝
Moreover, I think no re-review is needed so here is a 🟢
| }: { | ||
| children: React.ReactNode; | ||
| withoutCard?: boolean; | ||
| title: string; | ||
| subtitle?: JSX.Element; |
There was a problem hiding this comment.
I would prefer to use React.ReactNode over JSX.Element as React.ReactNode is more generic: From the react docs (according what the vscode hint showed me)
Represents all of the things React can render.
Where ReactElement only represents JSX, ReactNode represents everything that can be rendered.
| subtitle?: JSX.Element; | |
| subtitle?: React.ReactNode; |
URL of deployed dev instance (used for testing):
See feedback to first draft here: https://scm.slack.com/archives/C5AKLAV0B/p1737750240433629
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)