Keep your New GitHub issue draft after an accidental dismissal (#7778)

This commit is contained in:
Brennan Benson 2026-07-08 14:38:25 -07:00 committed by GitHub
parent 5a390cd60e
commit e218bf88f6
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 490 additions and 12 deletions

View File

@ -199,6 +199,12 @@ import {
type TaskPageRepoSourceState
} from '@/components/task-page-cache-selectors'
import { shouldHideTaskPageListChrome } from '@/components/task-page-list-chrome-visibility'
import {
isNewIssueDraftContentful,
resolveNewIssueOpenSeed,
resolveUserRepoSwitchReset,
resolveVanishedNewIssueRepoReset
} from '@/components/task-page-new-issue-draft'
import { findTaskPageJiraIssue } from '@/components/task-page-jira-cache-selectors'
import { getRepoBackedTaskEmptyState } from '@/components/task-page-empty-state'
import {
@ -4052,6 +4058,16 @@ export default function TaskPage(): React.JSX.Element {
const [newIssueAssignees, setNewIssueAssignees] = useState<GitHubAssignableUser[]>([])
const [newIssueSubmitting, setNewIssueSubmitting] = useState(false)
const [newIssueRepoId, setNewIssueRepoId] = useState<string | null>(null)
// Why: session-only draft slice backs recovery of an in-progress issue after
// an accidental dismissal (outside click/Escape/Cancel) and across a Tasks
// view unmount. Component `useState` stays the inputs' immediate source; the
// store is the durable-across-remount backing. See task-page-new-issue-draft.
// The draft value is read imperatively at open time (getState) rather than
// subscribed: it's only consumed in the `+` open handler, and the write-through
// rewrites it on every keystroke — subscribing would re-render all of TaskPage
// per keystroke while the modal is open. Actions are stable refs (no churn).
const setNewIssueDraft = useAppStore((s) => s.setNewIssueDraft)
const clearNewIssueDraft = useAppStore((s) => s.clearNewIssueDraft)
// Why: resolve the target repo from the user's choice, falling back to the
// first selected repo if the chosen id drops out of the selection while the
@ -4096,10 +4112,62 @@ export default function TaskPage(): React.JSX.Element {
{ runtimeEnvironmentId: newIssueOpen ? (newIssueRuntimeTarget?.environmentId ?? null) : null }
)
// Why: repo-scoped labels/assignees can't cross repos. A reactive clear keyed
// on the derived target id can't tell a restore apart from a user switch, so
// it would wipe just-restored fields and corrupt the recovery draft via the
// write-through below. Decompose by cause instead: this guard only handles the
// "chosen repo vanished from the selection" case (removed/deselected). A
// genuine user switch clears imperatively in the repo Select's handler; a
// restore always seeds an in-selection repoId, so neither path fires here.
useEffect(() => {
const reset = resolveVanishedNewIssueRepoReset(
newIssueRepoId,
selectedRepos.map((r) => r.id)
)
if (!reset) {
return
}
setNewIssueLabels([])
setNewIssueAssignees([])
}, [newIssueTargetRepo?.id])
setNewIssueRepoId(reset.repoId)
}, [newIssueRepoId, selectedRepos])
// Why: mirror the live fields into the session draft while the modal is open
// so an accidental dismissal doesn't lose input. Content-gate the write so an
// untouched open never pins a meaningless draft (repoId alone is not content),
// and clear any stale draft once the form is emptied back out.
useEffect(() => {
if (!newIssueOpen) {
return
}
if (
isNewIssueDraftContentful({
title: newIssueTitle,
body: newIssueBody,
labels: newIssueLabels,
assignees: newIssueAssignees
})
) {
setNewIssueDraft({
title: newIssueTitle,
body: newIssueBody,
labels: newIssueLabels,
assignees: newIssueAssignees,
repoId: newIssueRepoId
})
} else {
clearNewIssueDraft()
}
}, [
newIssueOpen,
newIssueTitle,
newIssueBody,
newIssueLabels,
newIssueAssignees,
newIssueRepoId,
setNewIssueDraft,
clearNewIssueDraft
])
const [selectedLinearIssueId, setSelectedLinearIssueId] = useState<string | null>(null)
const [selectedLinearIssueFallback, setSelectedLinearIssueFallback] =
@ -6633,6 +6701,10 @@ export default function TaskPage(): React.JSX.Element {
setNewIssueBody('')
setNewIssueLabels([])
setNewIssueAssignees([])
// Why: a successful submit is the only path that discards the recovery
// draft. Closing `newIssueOpen` in the same commit keeps the write-through
// effect (gated on it) from re-persisting the emptied fields.
clearNewIssueDraft()
// Why: bump the nonce so the list refetches and shows the new issue.
setTaskRefreshNonce((current) => current + 1)
@ -6700,7 +6772,8 @@ export default function TaskPage(): React.JSX.Element {
newIssueTargetRepo,
newIssueTitle,
openGitHubDetailPage,
setDialogWorkItem
setDialogWorkItem,
clearNewIssueDraft
])
const handleCreateNewLinearProject = useCallback(async (): Promise<void> => {
@ -8234,11 +8307,20 @@ export default function TaskPage(): React.JSX.Element {
variant="outline"
size="icon"
onClick={() => {
setNewIssueTitle('')
setNewIssueBody('')
setNewIssueLabels([])
setNewIssueAssignees([])
setNewIssueRepoId(primaryRepo?.id ?? null)
// Why: restore a content-non-empty draft instead
// of resetting, so an accidental dismissal is
// recoverable. The empty-default branch stays live
// so a stale draft never hijacks a fresh open after
// the user changed their primary/selected repo.
const seed = resolveNewIssueOpenSeed({
draft: useAppStore.getState().newIssueDraft,
selectedRepoIds: selectedRepos.map((r) => r.id)
})
setNewIssueTitle(seed.title)
setNewIssueBody(seed.body)
setNewIssueLabels(seed.labels)
setNewIssueAssignees(seed.assignees)
setNewIssueRepoId(seed.repoId)
setNewIssueOpen(true)
}}
disabled={!newIssueTargetRepo}
@ -11057,7 +11139,15 @@ export default function TaskPage(): React.JSX.Element {
</label>
<Select
value={newIssueRepoId ?? undefined}
onValueChange={(v) => setNewIssueRepoId(v)}
onValueChange={(v) => {
// Why: repo-scoped labels/assignees can't cross a genuine
// user repo switch, so clear them imperatively here (co-located
// with the cause). Restore never routes through this handler.
setNewIssueRepoId(v)
const reset = resolveUserRepoSwitchReset()
setNewIssueLabels(reset.labels)
setNewIssueAssignees(reset.assignees)
}}
disabled={newIssueSubmitting}
>
<SelectTrigger>

View File

@ -0,0 +1,165 @@
import { describe, expect, it } from 'vitest'
import {
isNewIssueDraftContentful,
resolveNewIssueOpenSeed,
resolveUserRepoSwitchReset,
resolveVanishedNewIssueRepoReset
} from './task-page-new-issue-draft'
import type { NewIssueDraft } from '@/store/slices/new-issue-draft'
import type { GitHubAssignableUser } from '../../../shared/types'
const assignee: GitHubAssignableUser = { login: 'octocat', name: 'Octo', avatarUrl: '' }
function draft(overrides: Partial<NewIssueDraft> = {}): NewIssueDraft {
return { title: '', body: '', labels: [], assignees: [], repoId: null, ...overrides }
}
describe('isNewIssueDraftContentful', () => {
it('is false for null and an all-empty draft', () => {
expect(isNewIssueDraftContentful(null)).toBe(false)
expect(isNewIssueDraftContentful(draft())).toBe(false)
})
it('is false for whitespace-only title/body', () => {
expect(isNewIssueDraftContentful(draft({ title: ' ', body: '\n\t' }))).toBe(false)
})
it('is false when only a repoId is set (repoId is not content)', () => {
expect(isNewIssueDraftContentful(draft({ repoId: 'repo-a' }))).toBe(false)
})
it('is true once a title, body, label, or assignee is present', () => {
expect(isNewIssueDraftContentful(draft({ title: 'Bug' }))).toBe(true)
expect(isNewIssueDraftContentful(draft({ body: 'details' }))).toBe(true)
expect(isNewIssueDraftContentful(draft({ labels: ['p1'] }))).toBe(true)
expect(isNewIssueDraftContentful(draft({ assignees: [assignee] }))).toBe(true)
})
})
describe('resolveNewIssueOpenSeed', () => {
it('takes the empty-default branch for no draft, targeting the first selected repo', () => {
expect(resolveNewIssueOpenSeed({ draft: null, selectedRepoIds: ['repo-a', 'repo-b'] })).toEqual(
{ title: '', body: '', labels: [], assignees: [], repoId: 'repo-a' }
)
})
it('takes the default branch for a draft carrying only a repoId', () => {
expect(
resolveNewIssueOpenSeed({
draft: draft({ repoId: 'repo-b' }),
selectedRepoIds: ['repo-a', 'repo-b']
})
).toEqual({ title: '', body: '', labels: [], assignees: [], repoId: 'repo-a' })
})
it('restores every field when the draft repo is still selected', () => {
expect(
resolveNewIssueOpenSeed({
draft: draft({
title: 'Bug',
body: 'details',
labels: ['p1'],
assignees: [assignee],
repoId: 'repo-b'
}),
selectedRepoIds: ['repo-a', 'repo-b']
})
).toEqual({
title: 'Bug',
body: 'details',
labels: ['p1'],
assignees: [assignee],
repoId: 'repo-b'
})
})
it('drops repo-scoped labels/assignees and falls back when the draft repo vanished', () => {
expect(
resolveNewIssueOpenSeed({
draft: draft({
title: 'Bug',
body: 'details',
labels: ['p1'],
assignees: [assignee],
repoId: 'removed-repo'
}),
selectedRepoIds: ['repo-a', 'repo-b']
})
).toEqual({
title: 'Bug',
body: 'details',
labels: [],
assignees: [],
repoId: 'repo-a'
})
})
it('resolves repoId to null only when nothing is selected', () => {
expect(
resolveNewIssueOpenSeed({ draft: draft({ title: 'Bug' }), selectedRepoIds: [] })
).toEqual({ title: 'Bug', body: '', labels: [], assignees: [], repoId: null })
})
})
describe('store-retention P1 guard (restore of an in-selection non-fallback repo)', () => {
it('never routes a valid restore through a scoped-field clear, so nothing empty is mirrored back', () => {
// P1 guard: a draft targeting a selected repo that is NOT selectedRepoIds[0]
// (the multi-repo case) must restore its repo-scoped labels/assignees intact.
// If restore instead emptied them, the write-through effect would mirror the
// empty values back into the store and durably corrupt the recovery draft.
// Compose the real helpers to prove restore never hits a clear path.
const original = draft({
title: 'Bug',
body: 'details',
labels: ['p1', 'regression'],
assignees: [assignee],
repoId: 'repo-b'
})
const selectedRepoIds = ['repo-a', 'repo-b']
const seed = resolveNewIssueOpenSeed({ draft: original, selectedRepoIds })
// Restore keeps the scoped fields and targets the draft's own repo...
expect(seed.repoId).toBe('repo-b')
expect(seed.repoId).not.toBe(selectedRepoIds[0])
expect(seed.labels).toEqual(['p1', 'regression'])
expect(seed.assignees).toEqual([assignee])
// ...and the vanish-guard does NOT fire on the restored (in-selection) repo,
// so no imperative clear runs either.
expect(resolveVanishedNewIssueRepoReset(seed.repoId, selectedRepoIds)).toBeNull()
// Therefore the fields the write-through would persist equal the original
// draft's scoped fields — non-empty and unchanged (no corruption).
expect(seed.labels.length).toBeGreaterThan(0)
expect(seed.assignees.length).toBeGreaterThan(0)
expect(seed.labels).toEqual(original.labels)
expect(seed.assignees).toEqual(original.assignees)
})
})
describe('resolveUserRepoSwitchReset', () => {
it('clears both repo-scoped labels and assignees on a genuine user repo switch', () => {
expect(resolveUserRepoSwitchReset()).toEqual({ labels: [], assignees: [] })
})
})
describe('resolveVanishedNewIssueRepoReset', () => {
it('returns null when the chosen repo is still selected', () => {
expect(resolveVanishedNewIssueRepoReset('repo-b', ['repo-a', 'repo-b'])).toBeNull()
})
it('returns null when no repo is chosen', () => {
expect(resolveVanishedNewIssueRepoReset(null, ['repo-a'])).toBeNull()
})
it('resets to the first selected repo when the chosen repo vanished', () => {
expect(resolveVanishedNewIssueRepoReset('removed', ['repo-a', 'repo-b'])).toEqual({
repoId: 'repo-a'
})
})
it('resets to null when the chosen repo vanished and nothing remains selected', () => {
expect(resolveVanishedNewIssueRepoReset('removed', [])).toEqual({ repoId: null })
})
})

View File

@ -0,0 +1,93 @@
import type { GitHubAssignableUser } from '../../../shared/types'
import type { NewIssueDraft } from '@/store/slices/new-issue-draft'
export type NewIssueOpenSeed = {
title: string
body: string
labels: string[]
assignees: GitHubAssignableUser[]
repoId: string | null
}
/** A draft is worth restoring only once it carries real user content a typed
* title/body or a picked label/assignee. A bare `repoId` (or an all-empty
* form) is NOT content, so an "opened but never edited" modal never pins a
* meaningless draft and never hijacks a fresh open. Shared by the write-through
* gate and the restore-on-open decision so both agree on "has content". */
export function isNewIssueDraftContentful(
draft: Pick<NewIssueDraft, 'title' | 'body' | 'labels' | 'assignees'> | null
): boolean {
if (!draft) {
return false
}
return (
draft.title.trim().length > 0 ||
draft.body.trim().length > 0 ||
draft.labels.length > 0 ||
draft.assignees.length > 0
)
}
/** Pure "seed vs. restore on open" decision for the New GitHub issue modal.
* - No content-non-empty draft empty defaults targeting the first selected
* repo (keeps a stale draft from hijacking a fresh open after the user
* changed their primary/selected repo).
* - Content draft whose repo is still selected restore every field.
* - Content draft whose repo vanished from the selection restore title/body
* only, drop the repo-scoped labels/assignees (they can't cross repos), and
* fall back to the first selected repo.
* Always resolves `repoId` to an explicit, in-selection id (never `null` while
* a target exists) so the vanish-guard can never misfire during a restore. */
export function resolveNewIssueOpenSeed(params: {
draft: NewIssueDraft | null
selectedRepoIds: readonly string[]
}): NewIssueOpenSeed {
const { draft, selectedRepoIds } = params
const fallbackRepoId = selectedRepoIds[0] ?? null
if (!draft || !isNewIssueDraftContentful(draft)) {
return { title: '', body: '', labels: [], assignees: [], repoId: fallbackRepoId }
}
if (draft.repoId !== null && selectedRepoIds.includes(draft.repoId)) {
return {
title: draft.title,
body: draft.body,
labels: draft.labels,
assignees: draft.assignees,
repoId: draft.repoId
}
}
return {
title: draft.title,
body: draft.body,
labels: [],
assignees: [],
repoId: fallbackRepoId
}
}
/** The reset patch to apply when the user picks a different repo in the
* selector: repo-scoped labels/assignees can't cross a genuine repo switch, so
* both are dropped. Kept as a named helper so the imperative `Select` handler
* and its test share one contract instead of an untested inline literal. */
export function resolveUserRepoSwitchReset(): {
labels: string[]
assignees: GitHubAssignableUser[]
} {
return { labels: [], assignees: [] }
}
/** Pure vanish-guard decision: when the chosen `newIssueRepoId` has left the
* selection (repo removed/deselected while a draft is open), returns the
* first-selected-repo fallback to reset the target to (the caller also drops
* the repo-scoped labels/assignees). Returns `null` when the chosen repo is
* still valid (or unset) no reset needed. Because a restore always seeds an
* in-selection `repoId`, this returns `null` during a valid restore. */
export function resolveVanishedNewIssueRepoReset(
newIssueRepoId: string | null,
selectedRepoIds: readonly string[]
): { repoId: string | null } | null {
if (newIssueRepoId === null || selectedRepoIds.includes(newIssueRepoId)) {
return null
}
return { repoId: selectedRepoIds[0] ?? null }
}

View File

@ -35,6 +35,7 @@ import { createRuntimeStatusSlice } from './slices/runtime-status'
import { createPullRequestGenerationSlice } from './slices/pull-request-generation'
import { createCommitMessageGenerationSlice } from './slices/commit-message-generation'
import { createPinnedTabCloseConfirmSlice } from './slices/pinned-tab-close-confirm'
import { createNewIssueDraftSlice } from './slices/new-issue-draft'
import { e2eConfig } from '@/lib/e2e-config'
import { registerHttpLinkStoreAccessor } from '@/lib/http-link-routing'
@ -73,7 +74,8 @@ export const useAppStore = create<AppState>()((...a) => ({
...createRuntimeStatusSlice(...a),
...createPullRequestGenerationSlice(...a),
...createCommitMessageGenerationSlice(...a),
...createPinnedTabCloseConfirmSlice(...a)
...createPinnedTabCloseConfirmSlice(...a),
...createNewIssueDraftSlice(...a)
}))
registerHttpLinkStoreAccessor(() => useAppStore.getState())

View File

@ -140,6 +140,7 @@ import { createRuntimeStatusSlice } from './runtime-status'
import { createPullRequestGenerationSlice } from './pull-request-generation'
import { createCommitMessageGenerationSlice } from './commit-message-generation'
import { createPinnedTabCloseConfirmSlice } from './pinned-tab-close-confirm'
import { createNewIssueDraftSlice } from './new-issue-draft'
function createTestStore() {
return create<AppState>()((...a) => ({
@ -177,7 +178,8 @@ function createTestStore() {
...createRuntimeStatusSlice(...a),
...createPullRequestGenerationSlice(...a),
...createCommitMessageGenerationSlice(...a),
...createPinnedTabCloseConfirmSlice(...a)
...createPinnedTabCloseConfirmSlice(...a),
...createNewIssueDraftSlice(...a)
}))
}

View File

@ -0,0 +1,81 @@
import { create } from 'zustand'
import { describe, expect, it } from 'vitest'
import { createNewIssueDraftSlice } from './new-issue-draft'
import type { AppState } from '../types'
import type { GitHubAssignableUser } from '../../../../shared/types'
function makeStore() {
return create<Pick<AppState, 'newIssueDraft' | 'setNewIssueDraft' | 'clearNewIssueDraft'>>()(
(...args) => createNewIssueDraftSlice(...(args as Parameters<typeof createNewIssueDraftSlice>))
)
}
const assignee: GitHubAssignableUser = { login: 'octocat', name: 'Octo', avatarUrl: '' }
describe('createNewIssueDraftSlice', () => {
it('starts with no draft', () => {
expect(makeStore().getState().newIssueDraft).toBeNull()
})
it('seeds a fresh empty draft with the patch when none exists', () => {
const store = makeStore()
store.getState().setNewIssueDraft({ title: 'Bug' })
expect(store.getState().newIssueDraft).toEqual({
title: 'Bug',
body: '',
labels: [],
assignees: [],
repoId: null
})
})
it('shallow-merges the patch into the current draft', () => {
const store = makeStore()
store.getState().setNewIssueDraft({
title: 'Bug',
body: 'details',
labels: ['p1'],
assignees: [assignee],
repoId: 'repo-a'
})
store.getState().setNewIssueDraft({ body: 'more details' })
expect(store.getState().newIssueDraft).toEqual({
title: 'Bug',
body: 'more details',
labels: ['p1'],
assignees: [assignee],
repoId: 'repo-a'
})
})
it('clears the draft back to null', () => {
const store = makeStore()
store.getState().setNewIssueDraft({ title: 'Bug', repoId: 'repo-a' })
store.getState().clearNewIssueDraft()
expect(store.getState().newIssueDraft).toBeNull()
})
it('seeds fresh label/assignee arrays per empty draft (no shared-constant aliasing)', () => {
// Regression: a module-level empty-draft constant would make partial patches
// that omit labels/assignees alias one shared array instance, so an in-place
// mutation of one draft would corrupt every subsequently-seeded empty draft.
const a = makeStore()
const b = makeStore()
a.getState().setNewIssueDraft({ title: 'A' })
b.getState().setNewIssueDraft({ title: 'B' })
expect(a.getState().newIssueDraft?.labels).not.toBe(b.getState().newIssueDraft?.labels)
expect(a.getState().newIssueDraft?.assignees).not.toBe(b.getState().newIssueDraft?.assignees)
a.getState().newIssueDraft?.labels.push('leak')
const c = makeStore()
c.getState().setNewIssueDraft({ title: 'C' })
expect(c.getState().newIssueDraft?.labels).toEqual([])
})
})

View File

@ -0,0 +1,41 @@
import type { StateCreator } from 'zustand'
import type { AppState } from '../types'
import type { GitHubAssignableUser } from '../../../../shared/types'
/** In-progress "New GitHub issue" composer draft. Session-only (never
* `persist`-wrapped, no disk surface): it exists so an accidental dismissal
* (outside click / Escape / Cancel) doesn't discard the user's input, and is
* cleared on a successful submit or app restart. */
export type NewIssueDraft = {
title: string
body: string
labels: string[]
assignees: GitHubAssignableUser[]
repoId: string | null
}
export type NewIssueDraftSlice = {
newIssueDraft: NewIssueDraft | null
/** Shallow-merge the patch into the current draft, or into a fresh empty
* draft when none exists yet. */
setNewIssueDraft: (patch: Partial<NewIssueDraft>) => void
clearNewIssueDraft: () => void
}
// Why: a factory (not a shared module constant) so a partial patch that omits
// `labels`/`assignees` seeds fresh arrays rather than aliasing one singleton's —
// an in-place mutation of an empty draft would otherwise corrupt every future one.
function createEmptyNewIssueDraft(): NewIssueDraft {
return { title: '', body: '', labels: [], assignees: [], repoId: null }
}
export const createNewIssueDraftSlice: StateCreator<AppState, [], [], NewIssueDraftSlice> = (
set
) => ({
newIssueDraft: null,
setNewIssueDraft: (patch) =>
set((state) => ({
newIssueDraft: { ...(state.newIssueDraft ?? createEmptyNewIssueDraft()), ...patch }
})),
clearNewIssueDraft: () => set({ newIssueDraft: null })
})

View File

@ -43,6 +43,7 @@ import { createRuntimeStatusSlice } from './runtime-status'
import { createPullRequestGenerationSlice } from './pull-request-generation'
import { createCommitMessageGenerationSlice } from './commit-message-generation'
import { createPinnedTabCloseConfirmSlice } from './pinned-tab-close-confirm'
import { createNewIssueDraftSlice } from './new-issue-draft'
import { translate } from '@/i18n/i18n'
export const TEST_REPO = {
@ -89,7 +90,8 @@ export function createTestStore() {
...createRuntimeStatusSlice(...a),
...createPullRequestGenerationSlice(...a),
...createCommitMessageGenerationSlice(...a),
...createPinnedTabCloseConfirmSlice(...a)
...createPinnedTabCloseConfirmSlice(...a),
...createNewIssueDraftSlice(...a)
}))
}

View File

@ -33,6 +33,7 @@ import type { RuntimeStatusSlice } from './slices/runtime-status'
import type { PullRequestGenerationSlice } from './slices/pull-request-generation'
import type { CommitMessageGenerationSlice } from './slices/commit-message-generation'
import type { PinnedTabCloseConfirmSlice } from './slices/pinned-tab-close-confirm'
import type { NewIssueDraftSlice } from './slices/new-issue-draft'
export type AppState = RepoSlice &
SparsePresetsSlice &
@ -68,4 +69,5 @@ export type AppState = RepoSlice &
RuntimeStatusSlice &
PullRequestGenerationSlice &
CommitMessageGenerationSlice &
PinnedTabCloseConfirmSlice
PinnedTabCloseConfirmSlice &
NewIssueDraftSlice