Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .github/instructions/pr-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ You are reviewing changes for @knighted/develop. Be concise, technical, and spec
## What to verify

- No generated artifacts are edited (dist/, coverage/, test-results/).
- Duplicated logic paths are avoided when a shared helper/module already exists; prefer reusing the established implementation.
- CDN import/fallback behavior is not bypassed with ad hoc URLs in feature modules.
- Sensitive values (PAT/token) are not logged or exposed in UI/status output.
- New UI behavior is covered in Playwright where appropriate.
Expand Down
6 changes: 6 additions & 0 deletions docs/idb-workspace-state.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ If a value is required to accurately restore PR/workspace behavior after reload,

`localStorage` should only mirror user preferences and lightweight bootstrap values.

PR metadata boundary:

- PR drawer form edits are draft-only input.
- Persisted PR metadata in workspace records is updated on successful workflow outcomes
(Open PR, Push Commit, Close/verified closed updates), not on each form edit.

## Post-Push Baseline Invariant

After a successful Push Commit action for an active PR workspace:
Expand Down
38 changes: 28 additions & 10 deletions docs/pr-context-storage-matrix.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ When the app loads, workspace restore scope depends on whether a repository is s
- If a repository is selected: use repository-scoped records only (`repo` match).
- If no repository is selected: evaluate all stored workspace records.

If a repository is selected and no repository-scoped records are found, restore falls back
to evaluating all stored workspace records.

Selection order:

1. Load candidate records using the scope above.
Expand All @@ -59,14 +62,33 @@ Selection order:
3. If preferred-by-id or preferred-by-key exists and is `active`, select it.
4. Otherwise select the first `active` record in candidates.
5. Otherwise select preferred-by-id or preferred-by-key if present.
6. Otherwise fall back to the first record returned by IDB ordering.
6. Otherwise fall back to the first candidate by recency (`lastModified` descending).

Notes:

- No `active workspace` pointer is stored in `localStorage`.
- Restore behavior is intentionally derived from IDB workspace records + in-memory runtime state.
- This avoids cross-storage drift between `localStorage` and IndexedDB.

## PR Drawer Persistence Boundary

PR drawer field edits are treated as draft input.

- Editing PR base/head/title in the drawer does not persist workspace record metadata by itself.
- Workspace PR metadata is committed by explicit successful workflow outcomes (for example:
successful Open PR, successful Push Commit, or successful Close PR/verified closed updates).

This keeps workspace records aligned to verified workflow outcomes instead of intermediate
form state.

## Active Record Model

Multiple `active` PR workspaces may exist for the same repository.

- The model does not enforce a single-active-record-per-repository invariant.
- Restore selection still follows the algorithm above and picks one workspace to load into
the current runtime.

## Why PR Context Lives In IDB Only

PR workflow state is part of workspace state.
Expand Down Expand Up @@ -101,14 +123,10 @@ indexedDB.open('knighted-develop-workspaces').onsuccess = event => {
}
```

## End-Of-Session Behavior

`Close` is the end-of-session action for PR-linked workspaces.

When close is confirmed:
## Close Behavior

1. The current workspace is archived as historical (`closed`).
2. The app immediately switches to a fresh local workspace (`inactive`) with a single empty entry tab.
3. Status messaging guides the user to continue locally or reopen a stored workspace from Workspaces.
`Close` archives PR-linked context in IDB and clears active PR context in runtime.

In the Workspaces drawer, inactive local-only workspace options are prefixed with `local:`.
- Matching workspace records for that PR context are persisted as `closed`.
- Close does not force an automatic fresh-local-workspace handoff.
- Workspace switching remains an explicit Workspaces action.
12 changes: 10 additions & 2 deletions playwright/github-pr-drawer/active-context-switch.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ test('Switching active workspace to closed preserves switched-from record integr
page,
targetState: 'closed',
})

await expect(page.getByRole('button', { name: 'Open pull request' })).toBeVisible()
await expect(
page.getByRole('button', { name: 'Close active pull request context' }),
).toBeHidden()

await expect(page.getByRole('status', { name: 'App status' })).toContainText('Rendered')
})

Expand Down Expand Up @@ -925,8 +931,7 @@ test('Active PR context updates controls and can be closed from AI controls', as
).toBeVisible()
await expect(
page.getByRole('list', { name: 'Workspace editor tabs' }).getByRole('listitem'),
).toHaveCount(1)
await expect(page.locator('#preview-host iframe')).toHaveCount(0)
).toHaveCount(2)

await expect
.poll(async () => {
Expand All @@ -941,12 +946,15 @@ test('Active PR context updates controls and can be closed from AI controls', as
return {
prContextState: closedRecord?.prContextState,
prNumber: closedRecord?.prNumber,
prTitle: closedRecord?.prTitle,
}
})
.toEqual({
prContextState: 'closed',
prNumber: 2,
prTitle: 'Existing PR context from storage',
})

expect(closePullRequestRequestCount).toBe(1)
})

Expand Down
5 changes: 1 addition & 4 deletions playwright/github-pr-drawer/github-pr-drawer.helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -926,10 +926,7 @@ export const runActiveWorkspaceSwitchIntegrityScenario = async ({
JSON.stringify(snapshot.active) === JSON.stringify(originalSnapshot.active)

const target = snapshot.target
const targetStateMatches =
targetState === 'closed'
? target?.prContextState === 'closed' || target?.prContextState === 'inactive'
: target?.prContextState === expectedTargetPrContextState
const targetStateMatches = target?.prContextState === expectedTargetPrContextState

const targetMatches =
target?.repo === originalSnapshot.target.repo &&
Expand Down
66 changes: 47 additions & 19 deletions src/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -812,15 +812,15 @@ const {
return
}

prDrawerController.clearSelectedRepositoryActivePrContext({ resetForm: false })

const nextWorkspaceRepositoryFullName =
typeof workspace.repo === 'string' ? workspace.repo.trim() : ''
if (nextWorkspaceRepositoryFullName) {
workspaceRepositoryFullName = nextWorkspaceRepositoryFullName
byotControls.setSelectedRepository(nextWorkspaceRepositoryFullName)
}

prDrawerController.clearSelectedRepositoryActivePrContext({ resetForm: false })

const state =
typeof workspace.prContextState === 'string'
? workspace.prContextState.trim().toLowerCase()
Expand Down Expand Up @@ -955,6 +955,7 @@ const workspacePrSessionHandoffController = createWorkspacePrSessionHandoffContr
defaults: {
defaultComponentTabName,
defaultComponentTabPath,
defaultComponentTabContent: defaultJsx,
},
state: {
getWorkspacePrNumber: () => workspacePrNumber,
Expand Down Expand Up @@ -998,17 +999,6 @@ const workspacePrSessionHandoffController = createWorkspacePrSessionHandoffContr
},
})

const archivePrSessionAndStartFreshLocal = ({ result, archivedState, statusMessage }) => {
hasObservedActivePrContextInSession = false
setWorkspacePrNumber(result?.pullRequestNumber)
byotControls.clearSelectedRepositoryPreference()
workspaceRepositoryFullName = ''
workspacePrSessionHandoffController.archivePrWorkspaceAndStartFreshLocal({
archivedState,
statusMessage,
})
}

const onPrContextStateChange = createPrContextStateChangeHandler({
toNonEmptyWorkspaceText,
toPullRequestNumber,
Expand Down Expand Up @@ -1120,16 +1110,25 @@ const githubWorkflows = createGitHubWorkflowsSetup({
if (nextPrNumber !== null) {
setWorkspacePrNumber(nextPrNumber)
}
persistWorkspacePrContextState('closed')
setWorkspacePrContextState('closed')

const persistClosedRecords = async () => {
const activeWorkspaceId = toNonEmptyWorkspaceText(activeWorkspaceRecordId)
const activeWorkspaceRecord = activeWorkspaceId
? await workspaceStorage.getWorkspaceById(activeWorkspaceId)
: null
const preservedPrTitle =
toNonEmptyWorkspaceText(activeWorkspaceRecord?.prTitle) ||
toNonEmptyWorkspaceText(githubPrTitle?.value)

await persistClosedPrContextRecords({
workspaceStorage,
selectedRepository: toNonEmptyWorkspaceText(
getCurrentSelectedRepositoryFullName(),
),
nextPrNumber,
normalizedHead: toNonEmptyWorkspaceText(githubPrHeadBranch?.value),
fallbackPrTitle: preservedPrTitle,
toNonEmptyWorkspaceText,
refreshLocalContextOptions,
})
Expand All @@ -1140,11 +1139,40 @@ const githubWorkflows = createGitHubWorkflowsSetup({
})
},
onPrContextClosed: result => {
archivePrSessionAndStartFreshLocal({
result,
archivedState: 'closed',
statusMessage:
'PR context closed. Open Workspaces to load a saved workspace or continue with this local workspace.',
hasObservedActivePrContextInSession = false
const nextPrNumber =
toPullRequestNumber(result?.pullRequestNumber) ??
parsePullRequestNumberFromUrl(result?.pullRequestUrl)
if (nextPrNumber !== null) {
setWorkspacePrNumber(nextPrNumber)
}
setWorkspacePrContextState('closed')

const persistClosedRecords = async () => {
const activeWorkspaceId = toNonEmptyWorkspaceText(activeWorkspaceRecordId)
const activeWorkspaceRecord = activeWorkspaceId
? await workspaceStorage.getWorkspaceById(activeWorkspaceId)
: null
const preservedPrTitle =
toNonEmptyWorkspaceText(result?.prTitle) ||
toNonEmptyWorkspaceText(activeWorkspaceRecord?.prTitle) ||
toNonEmptyWorkspaceText(githubPrTitle?.value)

await persistClosedPrContextRecords({
workspaceStorage,
selectedRepository: toNonEmptyWorkspaceText(
getCurrentSelectedRepositoryFullName(),
),
nextPrNumber,
normalizedHead: toNonEmptyWorkspaceText(githubPrHeadBranch?.value),
fallbackPrTitle: preservedPrTitle,
toNonEmptyWorkspaceText,
refreshLocalContextOptions,
})
}

void persistClosedRecords().catch(() => {
/* Save failures are already surfaced through saver onError. */
})
},
getPersistedActivePrContext,
Expand Down
59 changes: 51 additions & 8 deletions src/modules/app-core/github-workflows.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { toWorkspaceRecordKey } from '../workspace/workspace-tab-helpers.js'

const initializeGitHubWorkflows = ({
createGitHubPrEditorSyncController,
createGitHubChatDrawer,
Expand Down Expand Up @@ -228,13 +230,6 @@ const initializeGitHubWorkflows = ({
setSelectedRepository: setCurrentSelectedRepository,
getFileCommits: getWorkspacePrFileCommits,
getEditorSyncTargets,
persistWorkspaceMetadataOnSubmit: async () => {
if (typeof flushWorkspaceSave !== 'function') {
return
}

await flushWorkspaceSave({ preserveRecordId: true })
},
getTopLevelDeclarations,
getRenderMode,
getStyleMode,
Expand All @@ -249,6 +244,7 @@ const initializeGitHubWorkflows = ({
fileUpdates,
repositoryFullName,
pullRequestNumber,
branch,
}) => {
if (typeof onPrContextStateChange === 'function') {
onPrContextStateChange(githubAiContextState.activePrContext)
Expand Down Expand Up @@ -285,6 +281,28 @@ const initializeGitHubWorkflows = ({
activeWorkspaceRecordId,
)
if (activeWorkspaceRecord && typeof activeWorkspaceRecord === 'object') {
const nextHeadBranch =
typeof githubAiContextState.activePrContext?.headBranch === 'string' &&
githubAiContextState.activePrContext.headBranch.trim()
? githubAiContextState.activePrContext.headBranch.trim()
: typeof branch === 'string' && branch.trim()
? branch.trim()
: typeof activeWorkspaceRecord.head === 'string'
? activeWorkspaceRecord.head
: ''
Comment thread
knightedcodemonkey marked this conversation as resolved.
const nextBaseBranch =
typeof githubAiContextState.activePrContext?.baseBranch === 'string' &&
githubAiContextState.activePrContext.baseBranch.trim()
? githubAiContextState.activePrContext.baseBranch.trim()
: typeof activeWorkspaceRecord.base === 'string'
? activeWorkspaceRecord.base
: ''
const nextRepositoryFullName =
toSafeRepositoryFullName(repositoryFullName) ||
toSafeRepositoryFullName(
githubAiContextState.activePrContext?.repositoryFullName,
) ||
toSafeRepositoryFullName(activeWorkspaceRecord.repo)
const nextPrTitle =
typeof githubAiContextState.activePrContext?.prTitle === 'string' &&
githubAiContextState.activePrContext.prTitle.trim()
Expand All @@ -303,6 +321,14 @@ const initializeGitHubWorkflows = ({

const savedWorkspaceRecord = await workspaceStorage.upsertWorkspace({
...activeWorkspaceRecord,
workspaceScope: nextRepositoryFullName ? 'repository' : 'local',
workspaceKey: toWorkspaceRecordKey({
repositoryFullName: nextRepositoryFullName,
headBranch: nextHeadBranch,
}),
repo: nextRepositoryFullName,
base: nextBaseBranch,
head: nextHeadBranch,
prContextState: 'active',
prNumber: nextPrNumber,
prTitle: nextPrTitle,
Expand Down Expand Up @@ -544,8 +570,10 @@ const initializeGitHubWorkflows = ({
onPrContextStateChange(prDrawerController.getActivePrContext())
}

let isClosingActivePullRequest = false

githubPrContextClose?.addEventListener('click', () => {
if (!githubAiContextState.activePrContext) {
if (!githubAiContextState.activePrContext || isClosingActivePullRequest) {
return
}

Expand All @@ -559,6 +587,15 @@ const initializeGitHubWorkflows = ({
copy: `${referenceLine}PR title: ${githubAiContextState.activePrContext.prTitle}\nHead branch: ${githubAiContextState.activePrContext.headBranch}\n\nThis will close the pull request on GitHub and clear the active pull request context for the selected repository.`,
confirmButtonText: 'Close PR on GitHub',
onConfirm: () => {
if (isClosingActivePullRequest) {
return
}

isClosingActivePullRequest = true
if (githubPrContextClose instanceof HTMLButtonElement) {
githubPrContextClose.disabled = true
}

void prDrawerController
.closeActivePullRequestOnGitHub()
.then(result => {
Expand Down Expand Up @@ -586,6 +623,12 @@ const initializeGitHubWorkflows = ({
setStatus(`Close context failed: ${message}`, 'error')
showAppToast(`Close context failed: ${message}`)
})
.finally(() => {
isClosingActivePullRequest = false
if (githubPrContextClose instanceof HTMLButtonElement) {
githubPrContextClose.disabled = false
}
})
},
})
})
Expand Down
Loading
Loading