Skip to content

feat: BROS-596: Remove the top bar on the labeling stream #8822

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
+231 −430
Open

feat: BROS-596: Remove the top bar on the labeling stream #8822

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7b6f709
feat: BROS-596: Remove the top bar on the labeling stream
ricardoantoniocm Nov 12, 2025
0682dcd
Fix bem temporarily.
ricardoantoniocm Nov 13, 2025
76696f7
Moves CurrentTask component from TopBar to BottomBar. Hides TopBar on…
ricardoantoniocm Nov 13, 2025
ec5c559
Linter fixes.
ricardoantoniocm Nov 13, 2025
5448309
Merge branch 'develop' into 'fb-bros-596/remove-top-bar-labeling-stream'
ricardoantoniocm Nov 13, 2025
da4732d
Adds data-testids. Reverts order of logic in canPostpone.
ricardoantoniocm Nov 13, 2025
5242898
Removes unnecessary !p-0 from buttons.
ricardoantoniocm Nov 14, 2025
67f9246
Hide the CurrentTask component on Bulk Labeling mode.
ricardoantoniocm Nov 14, 2025
05577e0
Merge branch 'develop' into 'fb-bros-596/remove-top-bar-labeling-stream'
ricardoantoniocm Nov 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion web/libs/editor/src/components/AnnotationTab/AutoAcceptToggle.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
align-items: center;
box-sizing: border-box;
background-color: var(--color-neutral-background);
margin-left: 16px;
margin-left: var(--spacing-tight);

&__wrapper {
width: 100%;
Expand Down
52 changes: 27 additions & 25 deletions web/libs/editor/src/components/BottomBar/Actions.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import { IconInfoOutline, IconSettings } from "@humansignal/icons";
import { Button, Space } from "@humansignal/ui";
import { Button } from "@humansignal/ui";
import { cn } from "../../utils/bem";
import { isSelfServe } from "../../utils/billing";
import { FF_BULK_ANNOTATION, isFF } from "../../utils/feature-flags";
import { AutoAcceptToggle } from "../AnnotationTab/AutoAcceptToggle";
import { DynamicPreannotationsToggle } from "../AnnotationTab/DynamicPreannotationsToggle";
import { GroundTruth } from "../CurrentEntity/GroundTruth";
import { EditingHistory } from "./HistoryActions";
import "./Actions.scss";

export const Actions = ({ store }) => {
const annotationStore = store.annotationStore;
Expand All @@ -16,43 +17,44 @@ export const Actions = ({ store }) => {
const isBulkMode = isFF(FF_BULK_ANNOTATION) && !isSelfServe() && store.hasInterface("annotation:bulk");

return (
<Space size="small">
<div className={cn("bottombar").elem("section").toClassName()}>
{!isPrediction && !isViewAll && store.hasInterface("edit-history") && <EditingHistory entity={entity} />}

{store.description && store.hasInterface("instruction") && (
<div className={cn("action-buttons").toClassName()}>
{store.description && store.hasInterface("instruction") && (
<Button
type="text"
aria-label="Instructions"
size="small"
variant="neutral"
look="string"
tooltip="Show instructions"
onClick={() => store.toggleDescription()}
className="!p-0 aspect-square"
leading={<IconInfoOutline />}
/>
)}
<Button
type="text"
aria-label="Instructions"
aria-label="Settings"
size="small"
variant="neutral"
look="string"
tooltip="Show instructions"
onClick={() => store.toggleDescription()}
>
<IconInfoOutline />
</Button>
)}
<Button
type="text"
aria-label="Settings"
size="small"
look="string"
variant="neutral"
onClick={() => store.toggleSettings()}
tooltip="Settings"
className="!p-0"
>
<IconSettings />
</Button>
variant="neutral"
onClick={() => store.toggleSettings()}
tooltip="Settings"
className="!p-0aspect-square"
leading={<IconSettings />}
/>
</div>

{store.hasInterface("ground-truth") && !isBulkMode && <GroundTruth entity={entity} />}

{!isViewAll && (
<div className={cn("bottombar").elem("section").toClassName()}>
<div className={cn("model-actions").toClassName()}>
Copy link
Contributor

@bmartel bmartel Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this classname going to fail on testing selectors?

Copy link
Contributor Author

@ricardoantoniocm ricardoantoniocm Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probable. But I think the best approach would be to add data-testids to the buttons. I'll add those.

<DynamicPreannotationsToggle />
<AutoAcceptToggle />
</div>
)}
</Space>
</div>
);
};
40 changes: 40 additions & 0 deletions web/libs/editor/src/components/BottomBar/Actions.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
.action-buttons {
display: flex;
gap: var(--spacing-tight);
margin-left: var(--spacing-tight);

&__action {
width: 36px;
max-width: 36px;
height: 36px;

&:disabled {
opacity: 0.6;
}

svg {
display: block;
}
}
}

.model-actions {
padding: 0 var(--spacing-tight);
display: flex;
flex-wrap: nowrap;
gap: var(--spacing-tight);
position: relative;
margin-left: var(--spacing-tight);

&::before {
content: "";
display: block;
width: 1px;
height: calc(100% - var(--spacing-tight) * 2);
background-color: var(--color-neutral-border);
position: absolute;
left: 0;
top: var(--spacing-tight);
bottom: var(--spacing-tight);
}
}
2 changes: 2 additions & 0 deletions web/libs/editor/src/components/BottomBar/BottomBar.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { observer } from "mobx-react";
import { cn } from "../../utils/bem";
import { Actions } from "./Actions";
import { Controls } from "./Controls";
import { CurrentTask } from "./CurrentTask";
import "./BottomBar.scss";
import { FF_DEV_3873, isFF } from "../../utils/feature-flags";

Expand All @@ -18,6 +19,7 @@ export const BottomBar = observer(({ store }) => {
style={{ borderTop: isFF(FF_DEV_3873) && "1px solid rgba(0,0,0,0.1)" }}
>
<div className={cn("bottombar").elem("group").toClassName()}>
<CurrentTask store={store} />
<Actions store={store} />
</div>
<div className={cn("bottombar").elem("group").toClassName()}>
Expand Down
2 changes: 0 additions & 2 deletions web/libs/editor/src/components/BottomBar/BottomBar.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,8 @@

&__section {
display: flex;
padding: 0 var(--spacing-tight);
align-items: center;
box-sizing: border-box;
gap: var(--spacing-small);

&_flat {
padding: 0;
Expand Down
94 changes: 72 additions & 22 deletions web/libs/editor/src/components/BottomBar/CurrentTask.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,68 @@
import { useMemo } from "react";
import { observer } from "mobx-react";
import { Button, IconChevronLeft, IconChevronRight } from "@humansignal/ui";
import { useEffect, useState } from "react";
import { Button, IconChevronLeft, IconChevronRight, Tooltip } from "@humansignal/ui";
import { cn } from "../../utils/bem";
import { FF_DEV_3873, FF_DEV_4174, FF_LEAP_1173, FF_TASK_COUNT_FIX, isFF } from "../../utils/feature-flags";
import { guidGenerator } from "../../utils/unique";
import { isDefined } from "../../utils/utilities";
import { FF_LEAP_1173, FF_TASK_COUNT_FIX, isFF } from "../../utils/feature-flags";
import "./CurrentTask.scss";
import { reaction } from "mobx";

export const CurrentTask = observer(({ store }) => {
const currentIndex = useMemo(() => {
return store.taskHistory.findIndex((x) => x.taskId === store.task.id) + 1;
}, [store.taskHistory]);

const [initialCommentLength, setInitialCommentLength] = useState(0);
const [visibleComments, setVisibleComments] = useState(0);

useEffect(() => {
store.commentStore.setAddedCommentThisSession(false);

const reactionDisposer = reaction(
() => store.commentStore.comments.map((item) => item.isDeleted),
(result) => {
setVisibleComments(result.filter((item) => !item).length);
},
);

return () => {
reactionDisposer?.();
};
}, []);

useEffect(() => {
if (store.commentStore.addedCommentThisSession) {
setInitialCommentLength(visibleComments);
}
}, [store.commentStore.addedCommentThisSession]);
Comment on lines +17 to +39
Copy link
Contributor

@bmartel bmartel Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do?

Copy link
Collaborator

@hlomzik hlomzik Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically that's just a TopBar/CurrentTask.jsx moved to BottomBar. This file was not used at all, so it can be fully rewritten with no doubts. And apparently these files overlapped enough to have this specific diffs instead of full rewrite.

Copy link
Contributor Author

@ricardoantoniocm ricardoantoniocm Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of the code serves two purposes:

  1. Tracks visible (non-deleted) comments: The first useEffect sets up a MobX reaction that monitors the comment store. Whenever a comment's isDeleted status changes, it recalculates how many comments are still visible and updates the visibleComments state.
  2. Captures baseline comment count: The second useEffect watches for when a user adds their first comment in the current session (addedCommentThisSession flag). When this happens, it saves the current visible comment count as the initialCommentLength. This baseline is likely used to determine how many new comments were added during this session (by comparing current count to initial count).

This component needs to distinguish between comments that existed when the task was opened versus comments added during the current annotation session. This is used to help determine whether a task can be postponed. See lines 52-54.

When FF_DEV_4174 is enabled, the postpone functionality requires at least one "visible" comment.


const historyEnabled = store.hasInterface("topbar:prevnext");
const showCounter = store.hasInterface("topbar:task-counter");

// @todo some interface?
const canPostpone =
let canPostpone =
!isDefined(store.annotationStore.selected.pk) &&
!store.canGoNextTask &&
(!isFF(FF_LEAP_1173) || store.hasInterface("skip")) &&
!store.canGoNextTask &&
!store.hasInterface("review") &&
store.hasInterface("postpone");

if (store.hasInterface("annotations:comments") && isFF(FF_DEV_4174)) {
canPostpone = canPostpone && store.commentStore.addedCommentThisSession && visibleComments >= initialCommentLength;
}
Comment on lines +52 to +54
Copy link
Contributor

@bmartel bmartel Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure of this change, or why its necessary to move the topbar CurrentTask down to the bottombar.

Copy link
Contributor Author

@ricardoantoniocm ricardoantoniocm Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the context @bmartel https://humansignal.slack.com/archives/C016L55AYJX/p1762284677894759

Copy link
Contributor

@bmartel bmartel Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we changed the logic of canPostpone here. Maybe I am not seeing the ask in the messages.

Copy link
Contributor Author

@ricardoantoniocm ricardoantoniocm Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just about the moving the CurrentTask to the bottom. The canPostpone shouldn't have changed. I'll check that out.

Copy link
Collaborator

@hlomzik hlomzik Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here — it's just how it was in original TopBar file

Copy link
Contributor Author

@ricardoantoniocm ricardoantoniocm Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FF_DEV_4174 is specifically turned ON for just one of our customers:

https://app.launchdarkly.com/projects/default/flags/fflag_fix_back_dev_4174_overlap_issue_experiments_10012023_short/targeting?env=test&env=production&selected-env=production


return (
<div className={cn("bottombar").elem("section").toClassName()}>
<div className={cn("current-task").mod({ "with-history": historyEnabled }).toClassName()}>
<div className={cn("current-task").elem("task-id").toClassName()}>
<div
className={cn("current-task").elem("task-id").toClassName()}
style={{ fontSize: isFF(FF_DEV_3873) ? 12 : 14 }}
Copy link
Contributor

@bmartel bmartel Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar question here about the need for a FF based inline style

>
{store.task.id ?? guidGenerator()}
{historyEnabled &&
showCounter &&
(isFF(FF_TASK_COUNT_FIX) ? (
<div className={cn("current-task").elem("task-count").toClassName()}>
{store.queuePosition} of {store.queueTotal}
Expand All @@ -39,23 +74,38 @@ export const CurrentTask = observer(({ store }) => {
))}
</div>
{historyEnabled && (
<div className={cn("current-task").elem("history-controls").toClassName()}>
<Button
variant="neutral"
data-testid="prev-task"
disabled={!historyEnabled || !store.canGoPrevTask}
onClick={store.prevTask}
>
<IconChevronLeft />
</Button>
<Button
data-testid="next-task"
disabled={!store.canGoNextTask && !canPostpone}
onClick={store.canGoNextTask ? store.nextTask : store.postponeTask}
variant={!store.canGoNextTask && canPostpone ? "primary" : "neutral"}
>
<IconChevronRight />
</Button>
<div
className={cn("current-task")
.elem("history-controls")
.mod({ newui: isFF(FF_DEV_3873) })
Copy link
Contributor

@bmartel bmartel Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yyassi-heartex Do you recall what this FF_DEV_3873 is for? I feel there are a lot of changes in this PR that are hinging on its lingering existence, which makes it hard to understand what is necessary.

Copy link
Collaborator

@hlomzik hlomzik Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bmartel we are basically on UI v3 right now. We had v1 which was finally removed some time ago (FF_1170). And we still have v2 hidden behind feature flag. It's enabled for everyone and is ready to be removed, but it's everywhere, including some tests, so we have to fix them first. One day, one day...

.toClassName()}
>
<Tooltip title="Previous Task">
<Button
data-testid="prev-task"
aria-label="Previous task"
look="string"
disabled={!historyEnabled || !store.canGoPrevTask}
onClick={store.prevTask}
style={{ background: !isFF(FF_DEV_3873) && "none", backgroundColor: isFF(FF_DEV_3873) && "none" }}
Copy link
Contributor

@bmartel bmartel Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this FF_DEV_3873 gating, also why the inline style?

variant="neutral"
leading={<IconChevronLeft />}
size="small"
/>
</Tooltip>
<Tooltip title={!store.canGoNextTask && canPostpone ? "Postpone Task" : "Next Task"}>
<Button
data-testid="next-task"
aria-label="Next task"
look="string"
disabled={!store.canGoNextTask && !canPostpone}
onClick={store.canGoNextTask ? store.nextTask : store.postponeTask}
style={{ background: !isFF(FF_DEV_3873) && "none", backgroundColor: isFF(FF_DEV_3873) && "none" }}
variant={!store.canGoNextTask && canPostpone ? "primary" : "neutral"}
leading={<IconChevronRight />}
size="small"
/>
</Tooltip>
</div>
)}
</div>
Expand Down
Loading
Loading