Skip to content

Commit 7a64e59

Browse files
TheodoreSpeaksTheodoreSpeaks
committed
fix(issues): review round 2 — lock order, scoped review handoff, org menus, type inference
1 parent eaffc7c commit 7a64e59

13 files changed

Lines changed: 80 additions & 45 deletions

File tree

‎apps/docs/openapi-v2-resources.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1641,7 +1641,7 @@
16411641
"post": {
16421642
"operationId": "createIssue",
16431643
"summary": "Create Issue",
1644-
"description": "File an issue in a workspace. The body is the issue document as markdown; it lands in the inbox as new until someone starts work on it with Sim.\n\nOAuth scope: `api:write`.",
1644+
"description": "File an issue in a workspace. The body is the issue document as markdown; it lands in the inbox as new until someone starts work on it with Sim. Workspace API keys return `403`; use a personal API key or scoped OAuth token.\n\nOAuth scope: `api:write`.",
16451645
"x-sim-operation": "issues.create",
16461646
"x-oauth-scope": "api:write",
16471647
"tags": ["Issues"],
@@ -1712,7 +1712,7 @@
17121712
"get": {
17131713
"operationId": "getIssue",
17141714
"summary": "Get Issue",
1715-
"description": "Get one issue by its key, scoped to its workspace. Read the document with Get File Text on its bodyFileId.\n\nOAuth scope: `api:read`.",
1715+
"description": "Get one issue by its key, scoped to its workspace. Read the document with Read File Text on its bodyFileId. Workspace API keys return `403`; use a personal API key or scoped OAuth token.\n\nOAuth scope: `api:read`.",
17161716
"x-sim-operation": "issues.read",
17171717
"x-oauth-scope": "api:read",
17181718
"tags": ["Issues"],

‎apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/add-resource-dropdown/organization-resource-inventory.tsx‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,25 @@ import {
33
type AvailableResources,
44
useAvailableResources,
55
} from '@/app/workspace/[workspaceId]/home/components/mothership-view/components/add-resource-dropdown/available-resources'
6+
import type { MothershipResourceType } from '@/app/workspace/[workspaceId]/home/types'
67

78
/** Reuses each workspace's canonical inventory without introducing another picker. */
9+
interface OrganizationResourceInventoryProps {
10+
workspaceId: string
11+
onChange: (workspaceId: string, inventory: AvailableResources) => void
12+
/** Must be a module constant: it keys the inventory's group memo. */
13+
excludeTypes?: readonly MothershipResourceType[]
14+
}
15+
816
export function OrganizationResourceInventory({
917
workspaceId,
1018
onChange,
11-
}: {
12-
workspaceId: string
13-
onChange: (workspaceId: string, inventory: AvailableResources) => void
14-
}) {
15-
const inventory = useAvailableResources(workspaceId, { includeFolderMentions: true })
19+
excludeTypes,
20+
}: OrganizationResourceInventoryProps) {
21+
const inventory = useAvailableResources(workspaceId, {
22+
includeFolderMentions: true,
23+
excludeTypes,
24+
})
1625
useEffect(() => {
1726
onChange(workspaceId, inventory)
1827
}, [workspaceId, inventory, onChange])

‎apps/sim/app/workspace/[workspaceId]/home/components/user-input/components/plus-menu-dropdown/plus-menu-dropdown.tsx‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ const MENTION_ONLY_RESOURCE_TYPES = new Set<MothershipResourceType>(['integratio
8888
*/
8989
const WORKSPACE_SUBMENU_EXCLUDED_TYPES: readonly MothershipResourceType[] = [
9090
...MENTION_ONLY_RESOURCE_TYPES,
91+
...CHAT_CONTEXTLESS_RESOURCE_TYPES,
9192
'browser',
9293
'terminal',
9394
]
@@ -416,6 +417,7 @@ export const PlusMenuDropdown = React.memo(
416417
key={workspace.id}
417418
workspaceId={workspace.id}
418419
onChange={receiveInventory}
420+
excludeTypes={CHAT_CONTEXTLESS_RESOURCE_TYPES}
419421
/>
420422
))}
421423
<DropdownMenuTrigger asChild>

‎apps/sim/app/workspace/[workspaceId]/issues/components/issue-activity.tsx‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,11 @@ function CommentComposer({ workspaceId, issueKey }: CommentComposerProps) {
185185
const addComment = useAddIssueComment()
186186
const submit = () => {
187187
if (!body.trim() || addComment.isPending) return
188-
addComment.mutate({ workspaceId, key: issueKey, body }, { onSuccess: () => setBody('') })
188+
const submitted = body
189+
addComment.mutate(
190+
{ workspaceId, key: issueKey, body: submitted },
191+
{ onSuccess: () => setBody((current) => (current === submitted ? '' : current)) }
192+
)
189193
}
190194
return (
191195
<div className='flex flex-col gap-2'>

‎apps/sim/app/workspace/[workspaceId]/issues/issues.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ export function Issues({ workspaceId }: IssuesProps) {
6666
/>
6767
<div className='min-h-0 flex-1 overflow-y-auto'>
6868
<div className='mx-auto flex max-w-[860px] flex-col gap-10 px-6 py-8'>
69-
{query.error ? (
69+
{query.error && !query.data ? (
7070
<p role='alert' className='text-[var(--text-error)] text-small'>
7171
{query.error.message}
7272
</p>

‎apps/sim/lib/api/contracts/v2/openapi/resources.ts‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1133,8 +1133,7 @@ const declaredRoutes = [
11331133
applicationOperation: issueOperations.create,
11341134
operationId: 'createIssue',
11351135
summary: 'Create Issue',
1136-
description:
1137-
'File an issue in a workspace. The body is the issue document as markdown; it lands in the inbox as new until someone starts work on it with Sim.',
1136+
description: `File an issue in a workspace. The body is the issue document as markdown; it lands in the inbox as new until someone starts work on it with Sim. ${WORKSPACE_API_KEY_DENIED}`,
11381137
errors: RESOURCE_ERRORS,
11391138
success: { description: 'The issue was filed.' },
11401139
}),
@@ -1169,8 +1168,7 @@ const declaredRoutes = [
11691168
applicationOperation: issueOperations.read,
11701169
operationId: 'getIssue',
11711170
summary: 'Get Issue',
1172-
description:
1173-
'Get one issue by its key, scoped to its workspace. Read the document with Get File Text on its bodyFileId.',
1171+
description: `Get one issue by its key, scoped to its workspace. Read the document with Read File Text on its bodyFileId. ${WORKSPACE_API_KEY_DENIED}`,
11741172
errors: RESOURCE_ERRORS,
11751173
success: { description: 'The issue.' },
11761174
}),

‎apps/sim/lib/api/mcp/generated/v2-operations.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -640,7 +640,8 @@ export const V2_MCP_OPERATIONS = {
640640
contract: v2CreateIssueContract,
641641
summary: 'Create Issue',
642642
description:
643-
'File an issue in a workspace. The body is the issue document as markdown; it lands in the inbox as new until someone starts work on it with Sim.\n\nOAuth scope: `api:write`.',
643+
'File an issue in a workspace. The body is the issue document as markdown; it lands in the inbox as new until someone starts work on it with Sim. Workspace API keys return `403`; use a personal API key or scoped OAuth token.\n\nOAuth scope: `api:write`.',
644+
workspaceKeyUnsupported: true,
644645
handler: () => import('@/app/api/v2/issues/route').then((route) => route.POST),
645646
},
646647
createKnowledgeBase: {
@@ -1262,7 +1263,8 @@ export const V2_MCP_OPERATIONS = {
12621263
contract: v2GetIssueContract,
12631264
summary: 'Get Issue',
12641265
description:
1265-
'Get one issue by its key, scoped to its workspace. Read the document with Get File Text on its bodyFileId.\n\nOAuth scope: `api:read`.',
1266+
'Get one issue by its key, scoped to its workspace. Read the document with Read File Text on its bodyFileId. Workspace API keys return `403`; use a personal API key or scoped OAuth token.\n\nOAuth scope: `api:read`.',
1267+
workspaceKeyUnsupported: true,
12661268
handler: () => import('@/app/api/v2/issues/[issueKey]/route').then((route) => route.GET),
12671269
},
12681270
getKnowledgeBase: {

‎apps/sim/lib/issues/__integration__/issue-body-access.integration.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { mkdtempSync } from 'node:fs'
33
import { rm } from 'node:fs/promises'
44
import { tmpdir } from 'node:os'
55
import path from 'node:path'
6+
import type { Principal } from '@sim/auth/principal'
67
import { db, dbFor } from '@sim/db'
78
import { copilotChats, issue, issueEvent, organization, user, workspace } from '@sim/db/schema'
89
import { createSessionPrincipal, createWorkspaceApiKeyPrincipal } from '@sim/testing'
@@ -69,7 +70,7 @@ describe('issue bodies in PostgreSQL', () => {
6970
return created
7071
}
7172

72-
function readBody(fileId: string, principal = alice) {
73+
function readBody(fileId: string, principal: Principal = alice) {
7374
return readWorkspaceFileContent.execute({ principal, input: { fileId } })
7475
}
7576

‎apps/sim/lib/issues/application/issues.ts‎

Lines changed: 34 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -118,12 +118,13 @@ async function transition(
118118
values: Partial<IssueRow>,
119119
events: (row: IssueRow) => Omit<IssueEventInsert, 'issueId' | 'actor' | 'actorUserId'>[],
120120
principal: Principal,
121-
extra?: (tx: DbTransaction, row: IssueRow) => Promise<void>
121+
/** Runs before the issue row is locked, so every path takes the chat's lock before the issue's. */
122+
claimChat?: (tx: DbTransaction) => Promise<void>
122123
): Promise<IssueRow> {
123124
return db.transaction(async (tx) => {
125+
if (claimChat) await claimChat(tx)
124126
const row = await updateIssueInTx(tx, issueId, guard, values)
125127
if (!row) throw new OrchestrationError('conflict', 'The issue changed; reload it and try again')
126-
if (extra) await extra(tx, row)
127128
for (const event of events(row)) {
128129
await insertIssueEventInTx(tx, { ...eventBase(principal, issueId), ...event })
129130
}
@@ -155,14 +156,19 @@ async function presentIssue(issueId: string) {
155156
return record
156157
}
157158

158-
function auditOf(action: (typeof AuditAction)[keyof typeof AuditAction], description: string) {
159-
return ({ result }: { result: { issue: { id: string; key: string; title: string } } }) => ({
159+
/** Called from an unannotated `projectAudit` arrow, so it never narrows the inferred result. */
160+
function auditOf(
161+
action: (typeof AuditAction)[keyof typeof AuditAction],
162+
description: string,
163+
issue: { id: string; key: string; title: string }
164+
) {
165+
return {
160166
action,
161167
resourceType: AuditResourceType.ISSUE,
162-
resourceId: result.issue.id,
163-
resourceName: `${result.issue.key} ${result.issue.title}`,
168+
resourceId: issue.id,
169+
resourceName: `${issue.key} ${issue.title}`,
164170
description,
165-
})
171+
}
166172
}
167173

168174
export const listIssues = defineAuthorizedWorkspaceUseCase({
@@ -263,7 +269,7 @@ export const createIssue = defineAuthorizedWorkspaceUseCase({
263269
}
264270
},
265271
projectAudit: ({ result }) =>
266-
result.created ? auditOf(AuditAction.ISSUE_CREATED, 'Filed issue')({ result }) : [],
272+
result.created ? auditOf(AuditAction.ISSUE_CREATED, 'Filed issue', result.issue) : [],
267273
})
268274

269275
function isOpenFingerprintConflict(error: unknown): boolean {
@@ -322,13 +328,12 @@ export const updateIssue = defineAuthorizedWorkspaceUseCase({
322328
}
323329
return { issue: await presentIssue(current.id) }
324330
},
325-
projectAudit: (args) => {
326-
const { context, result } = args
331+
projectAudit: ({ context, result }) => {
327332
const changed =
328333
result.issue.title !== context.issue.title ||
329334
result.issue.priority !== context.issue.priority ||
330335
(result.issue.owner?.id ?? null) !== context.issue.ownerId
331-
return changed ? [auditOf(AuditAction.ISSUE_UPDATED, 'Updated issue')(args)] : []
336+
return changed ? [auditOf(AuditAction.ISSUE_UPDATED, 'Updated issue', result.issue)] : []
332337
},
333338
})
334339

@@ -360,12 +365,16 @@ export const startIssue = defineAuthorizedWorkspaceUseCase({
360365
principal,
361366
async (tx) => {
362367
if (!(await linkChatToIssueInTx(tx, input.chatId, context.issue.id)))
363-
throw new OrchestrationError('conflict', 'That chat already works on another issue')
368+
throw new OrchestrationError(
369+
'conflict',
370+
'That chat was deleted or already works on another issue'
371+
)
364372
}
365373
)
366374
return { issue: await presentIssue(context.issue.id) }
367375
},
368-
projectAudit: auditOf(AuditAction.ISSUE_WORK_STARTED, 'Started work on issue'),
376+
projectAudit: ({ result }) =>
377+
auditOf(AuditAction.ISSUE_WORK_STARTED, 'Started work on issue', result.issue),
369378
})
370379

371380
/** The working chat hands the issue back to its owner with a one-line result. */
@@ -376,11 +385,11 @@ export const requestIssueReview = defineAuthorizedWorkspaceUseCase({
376385
authorizationOptions,
377386
authorizeResource: ({ context }) => authorizeIssues(context),
378387
async execute({ input, context, principal }) {
379-
const callerChatId =
380-
principal.kind === 'delegated' && principal.serviceId === 'copilot'
381-
? principal.resourceScope?.chatId
382-
: undefined
383-
if (callerChatId !== undefined && callerChatId !== context.issue.workingChatId)
388+
// A delegated caller hands back only the issue its own chat is working on; no chat scope, no handoff.
389+
if (
390+
principal.kind === 'delegated' &&
391+
principal.resourceScope?.chatId !== context.issue.workingChatId
392+
)
384393
throw new OrchestrationError(
385394
'conflict',
386395
'Only the chat working on this issue can hand it back'
@@ -401,7 +410,8 @@ export const requestIssueReview = defineAuthorizedWorkspaceUseCase({
401410
)
402411
return { issue: await presentIssue(context.issue.id) }
403412
},
404-
projectAudit: auditOf(AuditAction.ISSUE_REVIEW_REQUESTED, 'Requested review'),
413+
projectAudit: ({ result }) =>
414+
auditOf(AuditAction.ISSUE_REVIEW_REQUESTED, 'Requested review', result.issue),
405415
})
406416

407417
export const approveIssue = defineAuthorizedWorkspaceUseCase({
@@ -419,7 +429,7 @@ export const approveIssue = defineAuthorizedWorkspaceUseCase({
419429
)
420430
return { issue: await presentIssue(context.issue.id) }
421431
},
422-
projectAudit: auditOf(AuditAction.ISSUE_CLOSED, 'Approved issue'),
432+
projectAudit: ({ result }) => auditOf(AuditAction.ISSUE_CLOSED, 'Approved issue', result.issue),
423433
})
424434

425435
/** Sends a reviewed issue back to its working chat. */
@@ -440,7 +450,8 @@ export const requestIssueChanges = defineAuthorizedWorkspaceUseCase({
440450
)
441451
return { issue: await presentIssue(context.issue.id) }
442452
},
443-
projectAudit: auditOf(AuditAction.ISSUE_CHANGES_REQUESTED, 'Requested changes'),
453+
projectAudit: ({ result }) =>
454+
auditOf(AuditAction.ISSUE_CHANGES_REQUESTED, 'Requested changes', result.issue),
444455
})
445456

446457
export interface CloseIssueInput extends IssueInput {
@@ -486,7 +497,7 @@ export const closeIssue = defineAuthorizedWorkspaceUseCase({
486497
)
487498
return { issue: await presentIssue(context.issue.id) }
488499
},
489-
projectAudit: auditOf(AuditAction.ISSUE_CLOSED, 'Closed issue'),
500+
projectAudit: ({ result }) => auditOf(AuditAction.ISSUE_CLOSED, 'Closed issue', result.issue),
490501
})
491502

492503
/** Reopens a closed issue as new; its earlier chats stay linked. */
@@ -512,7 +523,7 @@ export const reopenIssue = defineAuthorizedWorkspaceUseCase({
512523
)
513524
return { issue: await presentIssue(context.issue.id) }
514525
},
515-
projectAudit: auditOf(AuditAction.ISSUE_REOPENED, 'Reopened issue'),
526+
projectAudit: ({ result }) => auditOf(AuditAction.ISSUE_REOPENED, 'Reopened issue', result.issue),
516527
})
517528

518529
export interface IssueResourceInput extends IssueInput {

‎apps/sim/lib/issues/repository.integration.ts‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { readdirSync, readFileSync } from 'node:fs'
22
import { join } from 'node:path'
3-
import type * as schema from '@sim/db/schema'
3+
import * as schema from '@sim/db/schema'
44
import { readTestDatabaseUrl } from '@sim/db/testing/test-infrastructure'
55
import { withUtcTimestamps } from '@sim/db/timestamps'
66
import { generateId } from '@sim/utils/id'
@@ -46,22 +46,26 @@ function issueStatements(): string[] {
4646

4747
/** The trigger, checks and guards that keep `in_progress` tied to a live working chat. */
4848
describe('issue state in PostgreSQL', () => {
49-
const schema = `issue_repo_${generateId().replaceAll('-', '')}`
49+
const schemaName = `issue_repo_${generateId().replaceAll('-', '')}`
5050
const connection = postgres(
5151
readTestDatabaseUrl(),
52-
withUtcTimestamps({ max: 2, connection: { search_path: schema }, onnotice: () => undefined })
52+
withUtcTimestamps({
53+
max: 2,
54+
connection: { search_path: schemaName },
55+
onnotice: () => undefined,
56+
})
5357
)
5458

5559
beforeAll(async () => {
56-
await connection`CREATE SCHEMA ${connection(schema)}`
60+
await connection`CREATE SCHEMA ${connection(schemaName)}`
5761
await connection`CREATE TABLE copilot_chats (id uuid PRIMARY KEY)`
5862
for (const statement of issueStatements()) await connection.unsafe(statement)
5963
await connection`ALTER TABLE issue ADD FOREIGN KEY (working_chat_id) REFERENCES copilot_chats(id) ON DELETE SET NULL`
6064
database.current = drizzle(connection, { schema })
6165
})
6266

6367
afterAll(async () => {
64-
await connection`DROP SCHEMA ${connection(schema)} CASCADE`
68+
await connection`DROP SCHEMA ${connection(schemaName)} CASCADE`
6569
await connection.end()
6670
})
6771

0 commit comments

Comments
 (0)