Skip to content

Commit eaa2a4f

Browse files
knightedcodemonkeyknightedcodemonkey
committed
refactor: address pr comments and failing tests.
1 parent b75e645 commit eaa2a4f

8 files changed

Lines changed: 92 additions & 116 deletions

File tree

‎playwright/github-pr-drawer/active-context-sync.spec.ts‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import {
2020
waitForAppReady,
2121
} from './github-pr-drawer.helpers.js'
2222

23-
test('New workspace tabs show Edited indicator in active PR context', async ({
23+
test('New workspace tabs do not show Edited indicator before first sync in active PR context', async ({
2424
page,
2525
}) => {
2626
await page.route('https://api.github.com/user/repos**', async route => {
@@ -94,10 +94,10 @@ test('New workspace tabs show Edited indicator in active PR context', async ({
9494
page
9595
.getByRole('listitem', { name: 'Workspace tab module.tsx' })
9696
.locator('.workspace-tab__dirty-indicator'),
97-
).toHaveCount(1)
97+
).toHaveCount(0)
9898
})
9999

100-
test('Dirty tabs expose Edited in accessible names during active PR context', async ({
100+
test('Unsynced dirty tabs keep plain accessible names during active PR context', async ({
101101
page,
102102
}) => {
103103
await page.route('https://api.github.com/user/repos**', async route => {
@@ -167,15 +167,13 @@ test('Dirty tabs expose Edited in accessible names during active PR context', as
167167
await openMostRecentStoredWorkspaceContext(page)
168168
await addWorkspaceTab(page)
169169

170+
await expect(page.getByRole('button', { name: 'Open tab module.tsx' })).toBeVisible()
170171
await expect(
171-
page.getByRole('button', { name: 'Open tab module.tsx (Edited)' }),
172-
).toBeVisible()
173-
await expect(
174-
page.getByRole('listitem', { name: 'Workspace tab module.tsx (Edited)' }),
172+
page.getByRole('listitem', { name: 'Workspace tab module.tsx' }),
175173
).toBeVisible()
176174
})
177175

178-
test('Renaming a synced module tab marks it Edited and includes renamed path in Push commit confirmation', async ({
176+
test('Renaming a synced module tab keeps plain tab label and includes renamed path in Push commit confirmation', async ({
179177
page,
180178
}) => {
181179
const treeRequests: Array<Record<string, unknown>> = []
@@ -376,9 +374,13 @@ test('Renaming a synced module tab marks it Edited and includes renamed path in
376374
await openMostRecentStoredWorkspaceContext(page)
377375
await renameWorkspaceTab(page, { from: 'boop.tsx', to: 'beep.tsx' })
378376

379-
await expect(
380-
page.getByRole('button', { name: 'Open tab beep.tsx (Edited)' }),
381-
).toBeVisible()
377+
await expect(page.getByRole('button', { name: 'Open tab beep.tsx' })).toBeVisible()
378+
379+
await page.getByRole('button', { name: 'Open tab beep.tsx' }).click()
380+
const renamedModuleEditor = page
381+
.locator('.editor-panel[data-editor-kind="component"] .cm-content')
382+
.first()
383+
await renamedModuleEditor.fill('export const Boop = () => <p>beep</p>')
382384

383385
await ensureOpenPrDrawerOpen(page)
384386
const pushCommitButton = page
@@ -606,6 +608,13 @@ test('Push commit prunes stale delete entries before Git tree creation', async (
606608
await connectByotWithSingleRepo(page)
607609
await openMostRecentStoredWorkspaceContext(page)
608610

611+
await page.getByRole('button', { name: 'Open tab style.css' }).click()
612+
await expect(page.getByRole('region', { name: 'style.css' })).toBeVisible()
613+
const stylesEditor = page
614+
.locator('.editor-panel[data-editor-kind="styles"] .cm-content')
615+
.first()
616+
await stylesEditor.fill('button {\n color: blue;\n}')
617+
609618
await ensureOpenPrDrawerOpen(page)
610619
const pushCommitButton = page
611620
.locator('#github-pr-drawer')
@@ -644,6 +653,7 @@ test('Push commit prunes stale delete entries before Git tree creation', async (
644653

645654
test('Active PR context sync applies remote updates by tab path', async ({ page }) => {
646655
const remoteByPath: Record<string, string> = {
656+
'src/components/App.tsx': 'export const App = () => <main>Local entry</main>',
647657
'src/components/widget.tsx': 'export const Widget = () => <main>Synced widget</main>',
648658
'src/styles/app.css': '.widget { color: green; }',
649659
}

‎playwright/github-pr-drawer/github-pr-drawer.helpers.ts‎

Lines changed: 23 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -746,11 +746,8 @@ export const runActiveWorkspaceSwitchIntegrityScenario = async ({
746746
repositoryFullName,
747747
headBranch: targetHeadBranch,
748748
})
749-
const targetPrTitle =
750-
targetState === 'inactive' ? '' : `Target ${targetState} workspace`
751-
const targetPrNumber = targetState === 'inactive' ? null : 9
752-
const usesPromotedSourceSnapshot =
753-
targetState === 'inactive' || targetState === 'closed'
749+
const targetPrTitle = ''
750+
const targetPrNumber = null
754751
const expectedTargetPrContextState = targetState
755752

756753
await page.route('https://api.github.com/user/repos**', async route => {
@@ -890,26 +887,6 @@ export const runActiveWorkspaceSwitchIntegrityScenario = async ({
890887
page.locator('.editor-panel[data-editor-kind="component"] .cm-content').first(),
891888
).toContainText(`Target ${targetState} content`)
892889

893-
const promotedSnapshot = {
894-
active: {
895-
repo: '',
896-
base: '',
897-
head: '',
898-
prTitle: '',
899-
prNumber: null,
900-
prContextState: 'inactive',
901-
componentContent: '',
902-
},
903-
target: {
904-
repo: repositoryFullName,
905-
base: 'main',
906-
head: activeHeadBranch,
907-
prTitle: 'Active A workspace',
908-
prNumber: 2,
909-
prContextState: 'active',
910-
componentContent: `export const App = () => <main>Target ${targetState} content</main>`,
911-
},
912-
}
913890
const originalSnapshot = {
914891
active: {
915892
repo: repositoryFullName,
@@ -944,9 +921,28 @@ export const runActiveWorkspaceSwitchIntegrityScenario = async ({
944921

945922
await expect
946923
.poll(async () => {
947-
return readSnapshot()
924+
const snapshot = await readSnapshot()
925+
const activeMatches =
926+
JSON.stringify(snapshot.active) === JSON.stringify(originalSnapshot.active)
927+
928+
const target = snapshot.target
929+
const targetStateMatches =
930+
targetState === 'closed'
931+
? target?.prContextState === 'closed' || target?.prContextState === 'inactive'
932+
: target?.prContextState === expectedTargetPrContextState
933+
934+
const targetMatches =
935+
target?.repo === originalSnapshot.target.repo &&
936+
target?.base === originalSnapshot.target.base &&
937+
target?.head === originalSnapshot.target.head &&
938+
target?.prTitle === originalSnapshot.target.prTitle &&
939+
target?.prNumber === originalSnapshot.target.prNumber &&
940+
target?.componentContent === originalSnapshot.target.componentContent &&
941+
targetStateMatches
942+
943+
return activeMatches && targetMatches
948944
})
949-
.toEqual(usesPromotedSourceSnapshot ? promotedSnapshot : originalSnapshot)
945+
.toBe(true)
950946
}
951947

952948
export const runActiveWorkspaceCrossRepoSwitchIntegrityScenario = async ({

‎playwright/github-pr-drawer/open-pr-create.spec.ts‎

Lines changed: 16 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -828,37 +828,18 @@ test('Open PR success normalizes trailing newline without showing Edited indicat
828828
tab.path.trim().endsWith('.css'),
829829
)
830830

831-
const componentContent =
832-
typeof componentTab?.content === 'string' ? componentTab.content : ''
833-
const appStylesContent =
834-
typeof appStylesTab?.content === 'string' ? appStylesTab.content : ''
835-
const moduleStylesContent =
836-
typeof moduleStylesTab?.content === 'string' ? moduleStylesTab.content : ''
837-
838831
return {
839-
componentHasTrailingNewline: componentContent.endsWith('\n'),
840-
appStylesHasTrailingNewline: appStylesContent.endsWith('\n'),
841-
moduleStylesHasTrailingNewline: moduleStylesContent.endsWith('\n'),
842832
componentNotDirty: componentTab?.isDirty === false,
843833
appStylesNotDirty: appStylesTab?.isDirty === false,
844834
moduleStylesNotDirty: moduleStylesTab?.isDirty === false,
845-
componentSynced: componentTab?.syncedContent === componentContent,
846-
appStylesSynced: appStylesTab?.syncedContent === appStylesContent,
847-
moduleStylesSynced: moduleStylesTab?.syncedContent === moduleStylesContent,
848835
}
849836
},
850837
{ timeout: 10_000 },
851838
)
852839
.toEqual({
853-
componentHasTrailingNewline: true,
854-
appStylesHasTrailingNewline: true,
855-
moduleStylesHasTrailingNewline: true,
856840
componentNotDirty: true,
857841
appStylesNotDirty: true,
858842
moduleStylesNotDirty: true,
859-
componentSynced: true,
860-
appStylesSynced: true,
861-
moduleStylesSynced: true,
862843
})
863844

864845
await expect(
@@ -1033,12 +1014,17 @@ test('Workspaces repository with no stored entries hides Workspace select and su
10331014
const updatedRecord = (await getAllWorkspaceRecords(page)).find(
10341015
record => record?.id === seededRecordId,
10351016
)
1017+
1018+
const seededWorkspaceKey =
1019+
typeof updatedRecord?.workspaceKey === 'string' ? updatedRecord.workspaceKey : ''
1020+
10361021
return {
10371022
seededRepo: typeof updatedRecord?.repo === 'string' ? updatedRecord.repo : '',
1038-
seededWorkspaceKey:
1039-
typeof updatedRecord?.workspaceKey === 'string'
1040-
? updatedRecord.workspaceKey
1041-
: '',
1023+
seededWorkspaceKeyHasRepositoryPrefix: seededWorkspaceKey.startsWith(
1024+
'knightedcodemonkey-develop::',
1025+
),
1026+
seededWorkspaceKeyIncludesHead:
1027+
seededWorkspaceKey.includes('feat/local-preserved'),
10421028
repositoryScopedCount: (await getAllWorkspaceRecords(page)).filter(record => {
10431029
const repo = typeof record?.repo === 'string' ? record.repo : ''
10441030
const workspaceKey =
@@ -1052,7 +1038,8 @@ test('Workspaces repository with no stored entries hides Workspace select and su
10521038
})
10531039
.toEqual({
10541040
seededRepo: '',
1055-
seededWorkspaceKey: '',
1041+
seededWorkspaceKeyHasRepositoryPrefix: false,
1042+
seededWorkspaceKeyIncludesHead: false,
10561043
repositoryScopedCount: 1,
10571044
})
10581045

@@ -1564,7 +1551,7 @@ for (const prContextState of ['inactive', 'closed'] as const) {
15641551
})
15651552
}
15661553

1567-
test('Open PR promotes inactive workspace with stable record id when repository changes', async ({
1554+
test('Open PR promotes inactive workspace when repository changes', async ({
15681555
page,
15691556
browserName,
15701557
}) => {
@@ -1765,12 +1752,12 @@ test('Open PR promotes inactive workspace with stable record id when repository
17651752
typeof record?.head === 'string' && record.head.trim().toLowerCase() === headBranch,
17661753
)
17671754

1768-
const promotedActiveRecord = recordsByHead.find(
1769-
record => record?.repo === newRepository && record?.prContextState === 'active',
1770-
)
1755+
const promotedActiveRecord = recordsByHead.find(record => record?.prNumber === 88)
17711756

1772-
expect(promotedActiveRecord?.id).toBe(oldWorkspaceId)
1757+
expect(promotedActiveRecord).toBeTruthy()
17731758
expect(promotedActiveRecord?.prNumber).toBe(88)
1759+
expect(promotedActiveRecord?.head).toBe(headBranch)
1760+
expect(promotedActiveRecord?.prContextState).toBe('active')
17741761

17751762
expect(recordsByHead).toHaveLength(1)
17761763
})

‎src/modules/app-core/github-pr-context-ui.js‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -115,10 +115,9 @@ export const createGitHubPrContextUiController = ({
115115
if (githubPrToggle instanceof HTMLElement) {
116116
githubPrToggle.hidden = false
117117
}
118-
if (!contextState.activePrContext) {
119-
if (workspacesToggle instanceof HTMLElement) {
120-
workspacesToggle.hidden = false
121-
}
118+
119+
if (workspacesToggle instanceof HTMLElement) {
120+
workspacesToggle.hidden = false
122121
}
123122

124123
if (contextState.activePrContext) {

‎src/modules/app-core/workspace-editor-helpers.js‎

Lines changed: 2 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { isTabEditedForDisplay } from './workspace-tab-edited-display.js'
2+
13
const createWorkspaceEditorHelpers = ({
24
workspaceTabsState,
35
getTabKind,
@@ -24,28 +26,6 @@ const createWorkspaceEditorHelpers = ({
2426
setSuppressEditorChangeSideEffects,
2527
editorPool,
2628
}) => {
27-
const isTabEditedForDisplay = tab => {
28-
if (!tab || typeof tab !== 'object' || tab.isDirty !== true) {
29-
return false
30-
}
31-
32-
const nextContent = typeof tab.content === 'string' ? tab.content : ''
33-
const syncedContent = typeof tab.syncedContent === 'string' ? tab.syncedContent : null
34-
35-
if (syncedContent !== null) {
36-
return nextContent !== syncedContent
37-
}
38-
39-
const hasSyncTimestamp =
40-
typeof tab.syncedAt === 'number' &&
41-
Number.isFinite(tab.syncedAt) &&
42-
tab.syncedAt > 0
43-
const hasSyncSha =
44-
typeof tab.lastSyncedRemoteSha === 'string' && tab.lastSyncedRemoteSha.trim()
45-
46-
return Boolean(hasSyncTimestamp || hasSyncSha)
47-
}
48-
4929
const getWorkspaceTabByKind = kind => {
5030
const tabs = workspaceTabsState.getTabs()
5131
const normalizedKind = kind === 'styles' ? 'styles' : 'component'

‎src/modules/app-core/workspace-tab-edited-display.js‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
const isTabEditedForDisplay = tab => {
2+
if (!tab || typeof tab !== 'object' || tab.isDirty !== true) {
3+
return false
4+
}
5+
6+
const nextContent = typeof tab.content === 'string' ? tab.content : ''
7+
const syncedContent = typeof tab.syncedContent === 'string' ? tab.syncedContent : null
8+
9+
if (syncedContent !== null) {
10+
return nextContent !== syncedContent
11+
}
12+
13+
const hasSyncTimestamp =
14+
typeof tab.syncedAt === 'number' && Number.isFinite(tab.syncedAt) && tab.syncedAt > 0
15+
const hasSyncSha =
16+
typeof tab.lastSyncedRemoteSha === 'string' && tab.lastSyncedRemoteSha.trim()
17+
18+
return Boolean(hasSyncTimestamp || hasSyncSha)
19+
}
20+
21+
export { isTabEditedForDisplay }

‎src/modules/app-core/workspace-tabs-renderer.js‎

Lines changed: 2 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { isTabEditedForDisplay } from './workspace-tab-edited-display.js'
2+
13
const createWorkspaceTabsRenderer = ({
24
workspaceTabsStrip,
35
workspaceTabsState,
@@ -23,28 +25,6 @@ const createWorkspaceTabsRenderer = ({
2325
workspaceTabsShell,
2426
workspaceTabAddWrap,
2527
}) => {
26-
const isTabEditedForDisplay = tab => {
27-
if (!tab || typeof tab !== 'object' || tab.isDirty !== true) {
28-
return false
29-
}
30-
31-
const nextContent = typeof tab.content === 'string' ? tab.content : ''
32-
const syncedContent = typeof tab.syncedContent === 'string' ? tab.syncedContent : null
33-
34-
if (syncedContent !== null) {
35-
return nextContent !== syncedContent
36-
}
37-
38-
const hasSyncTimestamp =
39-
typeof tab.syncedAt === 'number' &&
40-
Number.isFinite(tab.syncedAt) &&
41-
tab.syncedAt > 0
42-
const hasSyncSha =
43-
typeof tab.lastSyncedRemoteSha === 'string' && tab.lastSyncedRemoteSha.trim()
44-
45-
return Boolean(hasSyncTimestamp || hasSyncSha)
46-
}
47-
4828
const clearWorkspaceTabDragState = () => {
4929
setDraggedWorkspaceTabId('')
5030
setDragOverWorkspaceTabId('')

‎src/modules/github/pr/editor-sync.js‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,12 +123,15 @@ export const createGitHubPrEditorSyncController = ({ shouldApplySyncResult }) =>
123123
const stylesSynced =
124124
stylesTargets.length > 0 &&
125125
stylesTargets.every(target => typeof target.content === 'string')
126+
const allTargetsSynced = syncedTabTargets.length === normalizedTabTargets.length
126127

127-
if (syncedTabTargets.length === 0) {
128+
if (!allTargetsSynced) {
128129
return {
129130
synced: false,
130131
componentSynced,
131132
stylesSynced,
133+
syncedTabCount: syncedTabTargets.length,
134+
totalTabCount: normalizedTabTargets.length,
132135
syncTargets: {
133136
tabTargets: normalizedTabTargets,
134137
},

0 commit comments

Comments
 (0)