diff --git a/config/scripts/check-changed-code-quality.mjs b/config/scripts/check-changed-code-quality.mjs index 71a512ad407..b7c73a669b0 100644 --- a/config/scripts/check-changed-code-quality.mjs +++ b/config/scripts/check-changed-code-quality.mjs @@ -13,6 +13,9 @@ const CASTING_DISABLE_PATTERN = /\/[/*]\s*(?:oxlint|eslint)-disable(?:-next-line|-line)?\s[^\n]*typescript\/consistent-type-assertions/ const ANTI_SLOP_DISABLE_PATTERN = /\/[/*]\s*(?:oxlint|eslint)-disable(?:-next-line|-line)?\s[^\n]*\banti-slop\// +const REACT_DOCTOR_DISABLE_PATTERN = + /^\s*\/[/*]\s*(?:oxlint|eslint)-disable(?:-next-line|-line)?\s+react-doctor\/[\w-]+(?:\s*,\s*react-doctor\/[\w-]+)*\s*(?:--(?:(?!\*\/).)*)?(?:\*\/)?\s*$/ +const EXPLICIT_DISABLE_RULE_PATTERN = /(?:-disable(?:-next-line|-line)?\s+|^)[\w-]+(?:\/[\w-]+)?/ export const OXLINT_SCANS = [ { // Why: no --config, so Oxlint keeps discovering nested configs. Pinning the root @@ -41,7 +44,12 @@ export const OXLINT_SCANS = [ }, { label: 'React Doctor', - args: ['--config', 'config/oxlint-react-doctor.json'] + args: [ + '--config', + 'config/oxlint-react-doctor.json', + '--report-unused-disable-directives-severity', + 'warn' + ] }, { // Why changed-lines only: the renderer carries ~4.7k pre-existing restyle/raw-color @@ -348,16 +356,34 @@ export function isCastingDirectiveUnusedWarning(diagnostic, root) { ) } -// Why: the anti-slop rules live in a JS plugin that only config/oxlint-anti-slop.json loads, so -// the root scan never sees those rule names and reports every anti-slop suppression as unused. -// `audit:anti-slop` is the scan that enforces them. -export function isAntiSlopDirectiveUnusedWarning(diagnostic, root) { +// Unloaded plugin directives are checked by their owning scan. +export function isUnloadedPluginDirectiveUnusedWarning(diagnostic, root, scanLabel) { if (!/^Unused (?:oxlint|eslint)-disable/.test(diagnostic.message ?? '')) { return false } + if (scanLabel === 'React Doctor') { + const labels = diagnostic.labels ?? [] + return ( + labels.length > 0 && + labels.every(({ span }) => { + if (span.offset === undefined || span.length === undefined) { + return false + } + const file = path.isAbsolute(diagnostic.filename) + ? diagnostic.filename + : path.join(root, diagnostic.filename) + // Oxlint spans use UTF-8 byte offsets, including before non-ASCII comments. + const directive = readFileSync(file) + .subarray(span.offset, span.offset + span.length) + .toString('utf8') + const rules = directive.split('--')[0] + return EXPLICIT_DISABLE_RULE_PATTERN.test(rules) && !/\breact-doctor\//.test(rules) + }) + ) + } return (diagnostic.labels ?? []).some((label) => - diagnosticHighlightedLines(root, diagnostic.filename, label.span).some((line) => - ANTI_SLOP_DISABLE_PATTERN.test(line) + diagnosticHighlightedLines(root, diagnostic.filename, label.span).some( + (line) => ANTI_SLOP_DISABLE_PATTERN.test(line) || REACT_DOCTOR_DISABLE_PATTERN.test(line) ) ) } @@ -436,7 +462,7 @@ export function main( (diagnostic) => !isSuppressedDiagnostic(diagnostic, root) && !isCastingDirectiveUnusedWarning(diagnostic, root) && - !isAntiSlopDirectiveUnusedWarning(diagnostic, root) && + !isUnloadedPluginDirectiveUnusedWarning(diagnostic, root, scan.label) && diagnosticTouchesAddedLines(diagnostic, rangesByFile, root, baseBlocks) ) for (const diagnostic of diagnostics) { diff --git a/config/scripts/check-changed-code-quality.test.mjs b/config/scripts/check-changed-code-quality.test.mjs index e722acad6ef..783ee140adc 100644 --- a/config/scripts/check-changed-code-quality.test.mjs +++ b/config/scripts/check-changed-code-quality.test.mjs @@ -1,10 +1,12 @@ import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' import path from 'node:path' import { describe, expect, it } from 'vitest' +import { runProcessSync } from '../../src/shared/child-process/run-process' +import { resolveOxlintInvocation } from './oxlint-cli-invocation.mjs' import { OXLINT_SCANS, diagnosticTouchesAddedLines, - isAntiSlopDirectiveUnusedWarning, + isUnloadedPluginDirectiveUnusedWarning, isMovedCode, isRootCodeQualityPath, overlapsAddedLines, @@ -124,7 +126,7 @@ describe('moved-code exemption', () => { }) }) -describe('anti-slop directive unused warning', () => { +describe('unloaded plugin directive unused warning', () => { const root = path.resolve(import.meta.dirname, '..', '..') // Assembled so no line here is itself a directive the gate would scan. const directive = (rule) => `/* oxlint-disable ${rule} -- reason */` @@ -146,21 +148,149 @@ describe('anti-slop directive unused warning', () => { it('exempts a suppression the root scan cannot resolve', () => { withFixture(directive('anti-slop/no-module-mocking'), (diagnostic) => { - expect(isAntiSlopDirectiveUnusedWarning(diagnostic, root)).toBe(true) + expect(isUnloadedPluginDirectiveUnusedWarning(diagnostic, root, 'code quality')).toBe(true) }) }) it('still reports an unused directive for a rule the root scan does load', () => { withFixture(directive('unicorn/no-array-reduce'), (diagnostic) => { - expect(isAntiSlopDirectiveUnusedWarning(diagnostic, root)).toBe(false) + expect(isUnloadedPluginDirectiveUnusedWarning(diagnostic, root, 'code quality')).toBe(false) }) }) it('ignores diagnostics that are not unused-directive warnings', () => { withFixture(directive('anti-slop/no-module-mocking'), (diagnostic) => { expect( - isAntiSlopDirectiveUnusedWarning({ ...diagnostic, message: 'Unexpected any.' }, root) + isUnloadedPluginDirectiveUnusedWarning( + { ...diagnostic, message: 'Unexpected any.' }, + root, + 'code quality' + ) ).toBe(false) }) }) + + function scanFixture(label, file) { + const scan = OXLINT_SCANS.find((candidate) => candidate.label === label) + if (!scan) { + throw new Error(`Missing ${label} scan`) + } + const { command, prefixArgs } = resolveOxlintInvocation(root) + const result = runProcessSync({ + program: command, + args: [...prefixArgs, ...scan.args, '--format', 'json', file], + cwd: root, + timeoutMs: 30_000, + maxOutputBytes: 4 * 1024 * 1024 + }) + return JSON.parse(result.stdout).diagnostics + } + + it('accepts a used Doctor directive only through its loaded scan', () => { + const source = [ + "import { useEffect, useState } from 'react'", + directive('react-doctor/no-derived-state-effect'), + 'export function Title({ title }: { title: string }) {', + " const [value, setValue] = useState('')", + ' useEffect(() => { setValue(title) }, [title])', + ' return value', + '}' + ].join('\n') + withFixture(source, ({ filename }) => { + const normal = scanFixture('code quality', filename) + const unused = normal.find((diagnostic) => diagnostic.message.startsWith('Unused ')) + expect(unused).toBeDefined() + expect(isUnloadedPluginDirectiveUnusedWarning(unused, root, 'code quality')).toBe(true) + expect(scanFixture('React Doctor', filename)).toEqual([]) + }) + }) + + it('keeps an unused Doctor directive failing in its loaded scan', () => { + withFixture(directive('react-doctor/no-derived-state-effect'), ({ filename }) => { + const diagnostics = scanFixture('React Doctor', filename) + expect(diagnostics).toHaveLength(1) + expect(diagnostics[0].message).toMatch(/^Unused /) + expect(isUnloadedPluginDirectiveUnusedWarning(diagnostics[0], root, 'React Doctor')).toBe( + false + ) + }) + }) + + it('does not hide unused native rules in a mixed directive', () => { + withFixture( + directive('react-doctor/no-derived-state-effect, unicorn/no-array-reduce'), + (diagnostic) => { + expect(isUnloadedPluginDirectiveUnusedWarning(diagnostic, root, 'code quality')).toBe(false) + } + ) + }) + + it('recognizes a standalone directive containing only Doctor rules', () => { + withFixture( + directive( + 'react-doctor/no-derived-state-effect, react-doctor/no-adjust-state-on-prop-change' + ), + (diagnostic) => { + expect(isUnloadedPluginDirectiveUnusedWarning(diagnostic, root, 'code quality')).toBe(true) + } + ) + }) + + it('keeps adjacent native directive warnings visible', () => { + const doctor = directive('react-doctor/no-derived-state-effect') + const native = directive('unicorn/no-array-reduce') + for (const source of [`${doctor} ${native}`, `${native} ${doctor}`]) { + withFixture(source, ({ filename }) => { + const diagnostic = scanFixture('code quality', filename).find((candidate) => + candidate.labels.some((label) => label.span.offset === source.indexOf(native)) + ) + expect(diagnostic).toBeDefined() + expect(diagnostic.message).toMatch(/^Unused /) + expect(isUnloadedPluginDirectiveUnusedWarning(diagnostic, root, 'code quality')).toBe(false) + }) + } + }) + + it('leaves used native directives to the scan that loads them', () => { + withFixture( + [ + 'export const banner = "λ"', + directive('typescript/no-explicit-any'), + 'export const answer: any = 42' + ].join('\n'), + ({ filename }) => { + expect(scanFixture('code quality', filename)).toEqual([]) + const diagnostics = scanFixture('React Doctor', filename) + expect(diagnostics).toHaveLength(1) + expect(isUnloadedPluginDirectiveUnusedWarning(diagnostics[0], root, 'React Doctor')).toBe( + true + ) + } + ) + }) + + it('does not exempt unused Doctor rules together with unloaded native rules', () => { + withFixture( + directive('react-doctor/no-derived-state-effect, typescript/no-explicit-any'), + ({ filename }) => { + const diagnostics = scanFixture('React Doctor', filename) + expect(diagnostics).toHaveLength(1) + expect(isUnloadedPluginDirectiveUnusedWarning(diagnostics[0], root, 'React Doctor')).toBe( + false + ) + } + ) + }) + + it('keeps blanket unused directives visible in the Doctor scan', () => { + for (const source of [directive(''), '// oxlint-disable-next-line -- reason']) { + withFixture(source, ({ filename }) => { + const diagnostics = scanFixture('React Doctor', filename) + expect(diagnostics).toHaveLength(1) + expect(isUnloadedPluginDirectiveUnusedWarning(diagnostics[0], root, 'React Doctor')).toBe( + false + ) + }) + } + }) }) diff --git a/src/renderer/src/components/JiraIssueWorkspace.tsx b/src/renderer/src/components/JiraIssueWorkspace.tsx index d423fe52799..6572692929a 100644 --- a/src/renderer/src/components/JiraIssueWorkspace.tsx +++ b/src/renderer/src/components/JiraIssueWorkspace.tsx @@ -95,11 +95,14 @@ export default function JiraIssueWorkspace({ [providerSettings] ) + // oxlint-disable-next-line react-doctor/no-derived-state-effect -- Why: seeds editable issue drafts while IPC hydration runs and invalidates obsolete requests. useEffect(() => { + requestIdRef.current += 1 if (!issue) { setFullIssue(null) setIssueLoading(false) setComments([]) + setCommentsLoading(false) setCommentsError(null) setTransitions([]) setPriorities([]) @@ -109,7 +112,6 @@ export default function JiraIssueWorkspace({ return } - requestIdRef.current += 1 const requestId = requestIdRef.current optimisticCommentsRef.current = [] setFullIssue(issue) diff --git a/src/renderer/src/components/jira-issue-workspace-request-lifetime.test.tsx b/src/renderer/src/components/jira-issue-workspace-request-lifetime.test.tsx new file mode 100644 index 00000000000..482098a4925 --- /dev/null +++ b/src/renderer/src/components/jira-issue-workspace-request-lifetime.test.tsx @@ -0,0 +1,296 @@ +// @vitest-environment happy-dom + +import { act, type ComponentProps, type ReactNode } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { JiraComment, JiraIssue } from '../../../shared/jira-types' +import type { TaskSourceContext } from '../../../shared/task-source-context' +import type { AppState } from '../store/types' +import type { + JiraIssueCommentComposer, + JiraIssueWorkspaceContent +} from './jira-issue-workspace-content' +import JiraIssueWorkspace from './JiraIssueWorkspace' + +type PendingRead = { + key: string + resolve: (value: T) => void + reject: (error: Error) => void +} +type ComposerProps = ComponentProps +type JiraStoreBoundary = Pick & { + settings: Pick, 'activeRuntimeEnvironmentId'> +} + +const boundary = vi.hoisted( + (): { + issues: PendingRead[] + comments: PendingRead[] + composer: ComposerProps | null + } => ({ issues: [], comments: [], composer: null }) +) + +vi.mock('@/store', () => { + const state: JiraStoreBoundary = { + settings: { activeRuntimeEnvironmentId: 'environment-1' }, + patchJiraIssue: () => {} + } + return { + useAppStore: (select: (state: JiraStoreBoundary) => unknown) => select(state) + } +}) +vi.mock('@/i18n/i18n', () => ({ translate: (_key: string, fallback: string) => fallback })) +vi.mock('@/runtime/runtime-jira-client', () => { + function request(queue: PendingRead[], key: string): Promise { + return new Promise((resolve, reject) => queue.push({ key, resolve, reject })) + } + return { + jiraGetIssue: (_settings: unknown, key: string) => request(boundary.issues, key), + jiraIssueComments: (_settings: unknown, key: string) => request(boundary.comments, key), + jiraListTransitions: async () => [], + jiraListPriorities: async () => [], + jiraListAssignableUsers: async () => [], + jiraUpdateIssue: async () => ({ ok: true }), + jiraAddIssueComment: async () => ({ ok: true, id: 'added' }) + } +}) +vi.mock('@/components/ui/sheet', () => { + const Content = ({ children }: { children: ReactNode }) =>
{children}
+ return { + // Keep the owner mounted, including its closed content, so stale state is observable. + Sheet: ({ open, children }: { open: boolean; children: ReactNode }) => ( +
{children}
+ ), + SheetContent: Content, + SheetTitle: Content, + SheetDescription: Content + } +}) +vi.mock('./jira-issue-workspace-chrome', () => ({ + JiraIssueWorkspaceHeader: ({ issueLoading }: { issueLoading: boolean }) => ( +
{String(issueLoading)}
+ ), + JiraIssueMetadataBar: () => null +})) +vi.mock('./jira-issue-workspace-content', () => ({ + JiraIssueWorkspaceContent: (props: ComponentProps) => ( +
+ {props.displayed.title} + {props.displayed.description} + {props.comments.map((comment) => comment.id).join(',')} + {String(props.commentsLoading)} + {props.commentsError} + +
+ ), + JiraIssueCommentComposer: (props: ComposerProps) => ( +
{ + boundary.composer = node ? props : null + }} + /> + ) +})) + +Object.assign(globalThis, { IS_REACT_ACT_ENVIRONMENT: true }) +const sourceContext: TaskSourceContext = { + kind: 'task-source', + provider: 'jira', + projectId: 'project-1', + hostId: 'runtime:environment-1' +} +let root: Root | null = null +let container: HTMLDivElement + +function issue(key = 'JIR-1', title = key): JiraIssue { + return { + id: key, + key, + title, + siteId: 'site-1', + description: '', + url: `https://jira.invalid/browse/${key}`, + project: { id: 'project', key: 'PRJ', name: 'Project' }, + issueType: { id: 'task', name: 'Task' }, + status: { id: 'open', name: 'Open', categoryKey: 'new', categoryName: 'New' }, + labels: [], + createdAt: '2026-10-02', + updatedAt: '2026-10-02' + } +} + +function comment(id: string): JiraComment { + return { id, body: id, createdAt: '2026-10-02' } +} + +async function render(selected: JiraIssue | null): Promise { + if (!root) { + throw new Error('Missing React root') + } + const mountedRoot = root + await act(async () => { + mountedRoot.render( + {}} + onClose={() => {}} + /> + ) + }) +} + +function take(queue: PendingRead[]): PendingRead { + const pending = queue.shift() + if (!pending) { + throw new Error('Missing pending Jira read') + } + return pending +} + +function text(id: string): string | null | undefined { + return container.querySelector(`[data-testid="${id}"]`)?.textContent +} + +async function retryComments(): Promise { + const button = container.querySelector('button') + if (!(button instanceof HTMLButtonElement)) { + throw new Error('Missing Retry button') + } + await act(async () => button.click()) +} + +beforeEach(() => { + boundary.issues.length = 0 + boundary.comments.length = 0 + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) +}) + +afterEach(async () => { + const mountedRoot = root + if (mountedRoot) { + await act(async () => mountedRoot.unmount()) + } + root = null + await act(async () => { + for (const pending of [...boundary.issues, ...boundary.comments]) { + pending.reject(new Error('Test cleanup')) + } + }) + boundary.issues.length = 0 + boundary.comments.length = 0 + boundary.composer = null + document.body.replaceChildren() +}) + +describe('Jira issue workspace request lifetime', () => { + it('discards late detail and comments after closing the mounted workspace', async () => { + await render(issue()) + await render(null) + await act(async () => { + take(boundary.issues).resolve({ ...issue(), description: 'Screenshot data'.repeat(32_768) }) + take(boundary.comments).resolve([{ ...comment('late'), body: 'Image data'.repeat(32_768) }]) + }) + expect(container.querySelector('[data-open]')?.getAttribute('data-open')).toBe('false') + expect(container.querySelector('[data-testid="detail"]')).toBeNull() + expect(boundary.composer).toBeNull() + }) + + it('clears an already hydrated issue and comments when it closes', async () => { + await render(issue()) + await act(async () => { + take(boundary.issues).resolve(issue('JIR-1', 'Hydrated')) + take(boundary.comments).resolve([comment('loaded')]) + }) + expect(text('title')).toBe('Hydrated') + expect(text('comments')).toBe('loaded') + expect(text('issue-loading')).toBe('false') + expect(text('comments-loading')).toBe('false') + await render(null) + expect(container.querySelector('[data-testid="detail"]')).toBeNull() + }) + + it('ignores old replies and accepts the reopened issue replies', async () => { + await render(issue()) + const oldIssue = take(boundary.issues) + const oldComments = take(boundary.comments) + await render(null) + await render(issue('JIR-2')) + await act(async () => { + oldIssue.resolve(issue('JIR-1', 'Late old issue')) + oldComments.resolve([comment('old')]) + }) + expect(text('title')).toBe('JIR-2') + expect(text('issue-loading')).toBe('true') + expect(text('comments-loading')).toBe('true') + await act(async () => { + take(boundary.issues).resolve(issue('JIR-2', 'New detail')) + take(boundary.comments).resolve([comment('new')]) + }) + expect(text('title')).toBe('New detail') + expect(text('comments')).toBe('new') + expect(text('comments-loading')).toBe('false') + }) + + it('discards a closed comments error together with late issue hydration', async () => { + await render(issue()) + await render(null) + await act(async () => { + take(boundary.comments).reject(new Error('Late failure')) + take(boundary.issues).resolve(issue('JIR-1', 'Late detail')) + }) + expect(container.querySelector('[data-testid="detail"]')).toBeNull() + await render(issue('JIR-2')) + expect(text('comments-error')).toBe('') + expect(text('comments-loading')).toBe('true') + }) + + it('keeps open comments errors retryable and clears loading on success', async () => { + await render(issue()) + await act(async () => { + take(boundary.issues).resolve(issue()) + take(boundary.comments).reject(new Error('Try again')) + }) + expect(text('comments-error')).toBe('Try again') + expect(text('comments-loading')).toBe('false') + await retryComments() + expect(text('comments-loading')).toBe('true') + expect(text('comments-error')).toBe('') + await act(async () => take(boundary.comments).resolve([comment('retry')])) + expect(text('comments')).toBe('retry') + expect(text('comments-loading')).toBe('false') + }) + + it('preserves submitted comments across refresh and deduplicates returned IDs', async () => { + await render(issue()) + await act(async () => { + take(boundary.issues).resolve(issue()) + take(boundary.comments).resolve([comment('server')]) + }) + await act(async () => { + if (!boundary.composer) { + throw new Error('Missing comment composer') + } + boundary.composer.setCommentDraft('Added comment') + }) + await act(async () => { + if (!boundary.composer) { + throw new Error('Missing comment composer') + } + boundary.composer.handleSubmitComment() + }) + expect(text('comments')).toBe('server,added') + await retryComments() + await act(async () => take(boundary.comments).resolve([comment('server')])) + expect(text('comments')).toBe('server,added') + await retryComments() + await act(async () => take(boundary.comments).resolve([comment('server'), comment('added')])) + expect(text('comments')).toBe('server,added') + expect(boundary.composer?.commentDraft).toBe('') + expect(boundary.composer?.commentSubmitting).toBe(false) + }) +})