Skip to content

feat: add per-task Revisit toggle and sidebar filter#2107

Open
sidjainn wants to merge 9 commits intoPostHog:mainfrom
sidjainn:feat/issue-2104-revisit-toggle
Open

feat: add per-task Revisit toggle and sidebar filter#2107
sidjainn wants to merge 9 commits intoPostHog:mainfrom
sidjainn:feat/issue-2104-revisit-toggle

Conversation

@sidjainn
Copy link
Copy Markdown

@sidjainn sidjainn commented May 8, 2026

Summary

Closes #2104.

Adds a per-task "Revisit" annotation so users can mark tasks they want to come back to later, plus a sidebar filter to focus on those tasks.

  • Sidebar right-click task options has a new Revisit toggle and Cmd+Shift+M shortcut (also listed in the keyboard shortcuts sheet).
  • Sidebar chat-bubble icon fills yellow on tasks marked for revisit (does not override functional state icons — permission, generating, cloud, suspended, unread, PR/diff, pinned).
  • Task list filter dropdown gains "All tasks / Revisit only" radio. Toggling back to "All tasks" returns to the default mode.
  • State persists per-device via a Zustand localStorage store (revisit-tasks-storage). Mirrors the pattern of other persisted sidebar prefs; pinned tasks remain server-side-backed and untouched by this change.
  • Filter narrowing applies to both pinned and unpinned task lists, so tasks pinned + marked for revisit still show under the pinned section, and non-revisit pinned tasks are hidden when the filter is on.

Analytics

Two new PostHog events captured (also fire when toggled via keyboard shortcut):

  • Task revisit toggled{ task_id, enabled }
  • Task revisit list filter changed{ filter_name, value, previous_value? }

Test plan

  • pnpm typecheck
  • pnpm lint
  • pnpm test (1181 passed; 15 new tests across revisitStore.test.ts and applyRevisitFilter.test.ts)
  • Manual (pnpm dev:code):
    • Open a task → Revisit toggle visible above the chat input on the right; off by default; tooltip shows the shortcut.
    • Flip on (Switch or Cmd+Shift+M) → sidebar chat bubble for that task turns yellow filled.
    • Reload the app → state persists.
    • Pinned / generating / suspended / PR-state tasks keep their existing icons (revisit does not override).
    • Open sidebar filter dropdown → choose "Revisit only" → list narrows to revisit-marked tasks. Choose "All tasks" → list restores.
    • Flip Revisit off on a task while the filter is "Revisit only" → task disappears from the list immediately.
    • PostHog events fire for both toggle (Switch + shortcut) and filter radio.

Created with PostHog Code

Siddharth Jain added 2 commits May 8, 2026 14:26
Closes PostHog#2104

Adds a per-task "Revisit" annotation so users can mark tasks they want
to come back to later.

- Inline Revisit Switch above the chat input on the right, with tooltip
  "Come back to revisit the task later (⇧⌘M)" and Cmd+Shift+M shortcut.
- Sidebar chat-bubble fills yellow on tasks marked for revisit.
- Task list filter dropdown gains "All tasks / Revisit only" radio so
  users can focus on tasks they flagged.
- State persists per-device via a Zustand localStorage store.
- Two PostHog events captured (also fire when toggled via shortcut):
  - "Task revisit toggled" { task_id, enabled }
  - "Task revisit list filter changed" { filter_name, value, previous_value }

Generated-By: PostHog Code
Task-Id: c63c2352-b983-4807-8b6c-029bb97ab9f5
- revisitStore: toggle, setRevisit (idempotent), persist + rehydrate.
- applyRevisitFilter: pure helper extracted from useSidebarData so the
  filter narrowing can be tested without mocking the full hook surface.

Generated-By: PostHog Code
Task-Id: c63c2352-b983-4807-8b6c-029bb97ab9f5
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 8, 2026

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
apps/code/src/renderer/features/task-detail/stores/revisitStore.ts:31-40
The `toggle` implementation duplicates the entire Set-copy-and-mutate pattern that already exists in `setRevisit`, violating OnceAndOnlyOnce. It can simply delegate to `setRevisit` by reading the current state with `get()`.

```suggestion
      toggle: (taskId) => get().setRevisit(taskId, !get().revisitTaskIds.has(taskId)),
```

### Issue 2 of 2
apps/code/src/renderer/features/task-detail/stores/revisitStore.test.ts:15-31
**Prefer parameterised tests for `setRevisit` variants**

The team rule is to always prefer parameterised tests. The `setRevisit(true)`, `setRevisit(false)`, and idempotency cases are variations of the same behaviour and could be consolidated into a single `it.each` table — e.g. parameterised over `[taskId, on, expectedSize]`. The same applies to `applyRevisitFilter.test.ts` which has several structurally similar cases (empty input, filter on/off, pinned) that could be grouped under `it.each`.

Reviews (1): Last reviewed commit: "test: add revisit store and revisit filt..." | Re-trigger Greptile

Comment thread apps/code/src/renderer/features/task-detail/stores/revisitStore.ts Outdated
Comment thread apps/code/src/renderer/features/task-detail/stores/revisitStore.test.ts Outdated
…e.ts

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 8, 2026

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
apps/code/src/renderer/features/sessions/components/SessionView.tsx:108
The store already exposes `isRevisit(taskId)` on its public interface precisely so callers don't need to reach into `revisitTaskIds` directly. Using `s.revisitTaskIds.has(taskId)` here (and again in `TaskListView.tsx`) duplicates the same expression that `isRevisit` encapsulates, breaking OnceAndOnlyOnce. The selector `(s) => s.isRevisit(taskId)` calls through to `get()` internally and behaves identically for Zustand's subscription machinery.

```suggestion
  const isRevisit = useRevisitStore((s) => s.isRevisit(taskId));
```

### Issue 2 of 2
apps/code/src/renderer/features/sidebar/components/TaskListView.tsx:108
Same OnceAndOnlyOnce issue: `s.revisitTaskIds.has(task.id)` duplicates the logic the store already encapsulates in `isRevisit`. Prefer `s.isRevisit(task.id)` to use the public helper consistently.

```suggestion
  const isRevisit = useRevisitStore((s) => s.isRevisit(task.id));
```

Reviews (2): Last reviewed commit: "Update apps/code/src/renderer/features/t..." | Re-trigger Greptile

@adboio adboio self-assigned this May 8, 2026
Consolidate setRevisit, rehydrate, and applyRevisitFilter cases into
it.each tables per team convention. Same coverage, fewer redundant
arrange/act/assert blocks.

Generated-By: PostHog Code
Task-Id: c63c2352-b983-4807-8b6c-029bb97ab9f5
@adboio
Copy link
Copy Markdown
Contributor

adboio commented May 8, 2026

@sidjainn thanks so much for opening this PR!!

i think this is great functionality - however, i wonder if it would be better slightly more "hidden" in the UI, so we don't have a persistent toggle on the tasks.

what do you think about instead of a toggle, adding this as an option in the right-click menu on tasks in the sidebar? so you could right click -> "mark as unread" or similar. same behavior, just without the toggle.

lmk your thoughts! :)

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 8, 2026

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
apps/code/src/renderer/features/task-detail/stores/revisitStore.test.ts:18
Superfluous method-name discriminant in ops type. The `"setRevisit"` literal is always discarded in the loop body (`for (const [, taskId, on] of ops)`), so it serves no purpose and violates simplicity rule #4 — no superfluous parts. Dropping it tightens the type and removes the silent dead element.

```suggestion
      ops: Array<[string, boolean]>;
```

### Issue 2 of 2
apps/code/src/renderer/features/task-detail/stores/revisitStore.test.ts:66-68
The loop skips the first element with a silent comma. If the type above is tightened to `[string, boolean]`, the destructuring can be simplified to remove the dead positional skip.

```suggestion
      for (const [taskId, on] of ops) {
        useRevisitStore.getState().setRevisit(taskId, on);
      }
```

Reviews (3): Last reviewed commit: "test: parameterise revisit store and fil..." | Re-trigger Greptile

@sidjainn
Copy link
Copy Markdown
Author

sidjainn commented May 8, 2026

@adboio yeah i think it is more graceful to keep the ux cleaner with all the non-critical knobs at 1 level down. i shall make the update, thanks for the feedback!

…o "Mark as unread"

Replaces the persistent toggle above the chat input with a right-click
menu item on each sidebar task ("Mark as unread" / "Unmark as unread").
Behavior is unchanged — yellow chat-bubble icon, sidebar filter, persisted
per-device — but the chrome above the chat input is gone.

- Add `mark-as-unread` action to the task context menu schema + service.
- `useTaskContextMenu` accepts `isMarkedAsUnread` + `onToggleMarkAsUnread`.
- `SidebarMenu` reads + toggles via `useRevisitStore`, fires analytics.
- Drop `RevisitToggleInline` from `SessionView`; keep the keyboard
  shortcut (Cmd+Shift+M) via a small `useMarkAsUnreadShortcut` hook so
  the active task can still be toggled without the right-click flow.
- Update copy: tooltip on the yellow icon now reads "Marked as unread";
  filter dropdown uses "Marked as unread only"; shortcut sheet entry is
  "Mark as unread"; shortcut constant renamed to `TOGGLE_MARK_AS_UNREAD`.
- Rename analytics events:
  - `TASK_REVISIT_TOGGLED` -> `TASK_MARK_AS_UNREAD_TOGGLED`
    ("Task mark as unread toggled")
  - `TASK_REVISIT_LIST_FILTER_CHANGED` -> `TASK_UNREAD_LIST_FILTER_CHANGED`
    ("Task unread list filter changed")

Internal store name (`revisitStore`) is kept to limit churn; only the
user-facing surface and event names change.

Generated-By: PostHog Code
Task-Id: c63c2352-b983-4807-8b6c-029bb97ab9f5
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 8, 2026

Comments Outside Diff (1)

  1. apps/code/src/shared/types/analytics.ts, line 801-823 (link)

    P1 Naming collision with the existing isUnread concept

    The PR introduces "Revisit" as its feature name (store: revisitStore, field: revisitTaskIds, prop: isRevisit), but every user-facing string and all analytics events use "unread" instead — TASK_MARK_AS_UNREAD_TOGGLED, TASK_UNREAD_LIST_FILTER_CHANGED, context-menu label "Mark as unread", keyboard constant TOGGLE_MARK_AS_UNREAD, filter label "Marked as unread only". PostHog already has a native task.isUnread concept (rendered as the unread indicator in the sidebar); naming this new feature's events and labels "unread" will cause confusion in dashboards that mix the two. Analytics event names are effectively write-once once data has been captured. The internal implementation already uses the right name (revisitTaskIds, showRevisitOnly); the user-facing strings and analytics events should match.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: apps/code/src/shared/types/analytics.ts
    Line: 801-823
    
    Comment:
    **Naming collision with the existing `isUnread` concept**
    
    The PR introduces "Revisit" as its feature name (store: `revisitStore`, field: `revisitTaskIds`, prop: `isRevisit`), but every user-facing string and all analytics events use "unread" instead — `TASK_MARK_AS_UNREAD_TOGGLED`, `TASK_UNREAD_LIST_FILTER_CHANGED`, context-menu label "Mark as unread", keyboard constant `TOGGLE_MARK_AS_UNREAD`, filter label "Marked as unread only". PostHog already has a native `task.isUnread` concept (rendered as the unread indicator in the sidebar); naming this new feature's events and labels "unread" will cause confusion in dashboards that mix the two. Analytics event names are effectively write-once once data has been captured. The internal implementation already uses the right name (`revisitTaskIds`, `showRevisitOnly`); the user-facing strings and analytics events should match.
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
apps/code/src/shared/types/analytics.ts:801-823
**Naming collision with the existing `isUnread` concept**

The PR introduces "Revisit" as its feature name (store: `revisitStore`, field: `revisitTaskIds`, prop: `isRevisit`), but every user-facing string and all analytics events use "unread" instead — `TASK_MARK_AS_UNREAD_TOGGLED`, `TASK_UNREAD_LIST_FILTER_CHANGED`, context-menu label "Mark as unread", keyboard constant `TOGGLE_MARK_AS_UNREAD`, filter label "Marked as unread only". PostHog already has a native `task.isUnread` concept (rendered as the unread indicator in the sidebar); naming this new feature's events and labels "unread" will cause confusion in dashboards that mix the two. Analytics event names are effectively write-once once data has been captured. The internal implementation already uses the right name (`revisitTaskIds`, `showRevisitOnly`); the user-facing strings and analytics events should match.

### Issue 2 of 2
apps/code/src/renderer/features/sessions/components/SessionView.tsx:103-121
**Switch UI missing from `SessionView`**

The PR description states: *"Inline Revisit `Switch` above the chat input on the right, with tooltip 'Come back to revisit the task later (⇧⌘M)'"*. The diff for `SessionView.tsx` only adds the keyboard shortcut hook; there is no `Switch` component or tooltip in the JSX. The manual checklist in the PR is also unchecked for this item. Without a visible toggle, users have no way to discover or operate the feature except via the context menu or the undiscoverable keyboard shortcut.

Reviews (4): Last reviewed commit: "refactor: move revisit toggle into sideb..." | Re-trigger Greptile

Comment thread apps/code/src/renderer/features/sessions/components/SessionView.tsx Outdated
…cement

Reverts the user-facing copy from "Mark as unread" to "Revisit" since
the unread phrasing collided with the existing isUnread state (green
dot on tasks with new activity since last viewed). The right-click
menu placement stays — there is no persistent toggle above the chat.

- Context menu item: "Mark for revisit" / "Unmark for revisit"
- Schema action type: mark-as-unread -> toggle-revisit
- Schema input flag: isMarkedAsUnread -> isRevisit
- Hook callback: onToggleMarkAsUnread -> onToggleRevisit
- Tooltip on the yellow chat-bubble icon: "Marked for revisit"
- Filter dropdown radio: "Revisit only"
- Shortcut constant: TOGGLE_REVISIT (Cmd+Shift+M kept)
- Shortcut sheet entry: "Toggle revisit task"
- SessionView shortcut hook: useRevisitShortcut
- Analytics events:
  - TASK_REVISIT_TOGGLED ("Task revisit toggled")
  - TASK_REVISIT_LIST_FILTER_CHANGED ("Task revisit list filter changed")

Generated-By: PostHog Code
Task-Id: c63c2352-b983-4807-8b6c-029bb97ab9f5
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 8, 2026

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
apps/code/src/renderer/features/sessions/components/SessionView.tsx:103-121
Unnecessary store subscription broadens re-renders — `revisitTaskIds` is subscribed here so that `wasMarked` can be read inside the hotkeys callback, but the subscription fires a re-render of `SessionView` every time **any** task's revisit status changes, not just the current task. Reading from `getState()` directly inside the handler avoids the subscription entirely while keeping the value fresh at key-press time.

```suggestion
function useRevisitShortcut(taskId: string | undefined) {
  const toggle = useRevisitStore((s) => s.toggle);
  useHotkeys(
    SHORTCUTS.TOGGLE_REVISIT,
    (e) => {
      if (!taskId) return;
      e.preventDefault();
      const wasMarked = useRevisitStore.getState().revisitTaskIds.has(taskId);
      toggle(taskId);
      track(ANALYTICS_EVENTS.TASK_REVISIT_TOGGLED, {
        task_id: taskId,
        enabled: !wasMarked,
      });
    },
    { enableOnFormTags: true, enableOnContentEditable: true },
    [taskId, toggle],
  );
}
```

Reviews (5): Last reviewed commit: "refactor: rename mark-as-unread back to ..." | Re-trigger Greptile

Adds checkbox-style menu items to the platform context-menu primitive
and uses it for the per-task Revisit toggle. The right-click menu now
shows a single "Revisit" entry with a check mark when the task is
marked, instead of swapping the label between "Mark for revisit" and
"Unmark for revisit". Mirrors the on/off feel of the original switch
that lived above the chat input.

- Add `checked?: boolean` to `ContextMenuAction` and `ActionItemDef`.
- Pass through to Electron's `MenuItemConstructorOptions` as
  `type: "checkbox"` + `checked`.
- Use `checked: isRevisit` for the Revisit menu item; label is now
  just "Revisit".

Generated-By: PostHog Code
Task-Id: c63c2352-b983-4807-8b6c-029bb97ab9f5
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 8, 2026

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
apps/code/src/renderer/features/sessions/components/SessionView.tsx:103-121
**`SessionView` re-renders on every revisit state change across all tasks**

`useRevisitShortcut` subscribes to the entire `revisitTaskIds` Set via `useRevisitStore((s) => s.revisitTaskIds)`. Because `setRevisit` always constructs a new `Set`, any task being toggled anywhere in the app produces a new reference and triggers a full `SessionView` re-render — even when the toggled task has nothing to do with the currently open session. Replacing the Set subscription with a narrower selector (e.g. `(s) => s.revisitTaskIds.has(taskId ?? '')`) and reading the full state inside the handler via `useRevisitStore.getState()` at call time would limit re-renders to only when the current task's own state changes.

### Issue 2 of 2
apps/code/src/renderer/features/tasks/hooks/useArchiveTask.ts:20-40
**Archived task IDs not cleared from revisit store**

`archiveTaskImperative` cleans up pinned, terminal, and command-center state for the task, but it does not call `useRevisitStore.getState().setRevisit(taskId, false)`. Two consequences: (1) the `revisitTaskIds` Set in localStorage grows without bound as tasks are archived over time; (2) if a task is later unarchived, it silently re-appears as marked for revisit in the sidebar, which may surprise the user. Adding a single `setRevisit(taskId, false)` call alongside the other cleanup calls would keep the store consistent.

Reviews (6): Last reviewed commit: "feat: render Revisit context-menu item a..." | Re-trigger Greptile

…tore

- useRevisitShortcut no longer subscribes to revisitTaskIds; reads via
  useRevisitStore.getState() inside the keypress handler. Prevents the
  whole SessionView from re-rendering whenever any task's revisit state
  is toggled elsewhere.
- archiveTaskImperative now clears the archived task from the revisit
  store alongside the existing pin/terminal/command-center cleanup, so
  the persisted revisitTaskIds Set does not grow unboundedly and an
  unarchived task does not silently come back marked for revisit.

Generated-By: PostHog Code
Task-Id: c63c2352-b983-4807-8b6c-029bb97ab9f5
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 8, 2026

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
apps/code/src/renderer/features/tasks/hooks/useTasks.ts:251-253
**Missing revisit cleanup on delete**

`archiveTaskImperative` correctly calls `setRevisit(taskId, false)` when a task is archived, but `deleteWithConfirm` does not. Deleted task IDs will accumulate indefinitely in the `revisit-tasks-storage` localStorage key. While the stale IDs are inert (the deleted task never appears in the task list), the store grows without bound and mirrors the existing `pinnedTasksApi.unpin(taskId)` call that is already present here.

```suggestion
      pinnedTasksApi.unpin(taskId);
      useRevisitStore.getState().setRevisit(taskId, false);

      await mutation.mutateAsync(taskId);
```

Reviews (7): Last reviewed commit: "fix: address SessionView re-renders and ..." | Re-trigger Greptile

Mirrors the unpin/clear-revisit cleanup that already runs on archive,
so the persisted revisit-tasks-storage Set does not accumulate IDs of
deleted tasks.

Generated-By: PostHog Code
Task-Id: c63c2352-b983-4807-8b6c-029bb97ab9f5
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 8, 2026

Reviews (8): Last reviewed commit: "fix: clear revisit state when deleting a..." | Re-trigger Greptile

@sidjainn
Copy link
Copy Markdown
Author

sidjainn commented May 8, 2026

@adboio i have moved the revisit feature to right-click on the task in the sidebar. avoided the "mark as unread" copy because of 2 reasons - conflict with the green dot copy, and secondly mark as unread implies that clicking on the task will make it read but i feel revisit is more intentional and it is better to explicity remove a task from revisit list. lmk thoughts if any
Screenshot 2026-05-08 at 11 22 25 PM

@sidjainn
Copy link
Copy Markdown
Author

sidjainn commented May 8, 2026

There is one stylistic difference - there's a tick that shows when a task is marked for revisit. User can click on the option again to remove the task from revisit (it's quite intuitive). The other option would be to change copy, like pin and unpin, for example. i found the former better because it's easier to scan what is ticked, scanning through copy change entails higher effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add revisit toggle to come back to a chat

2 participants