Skip to content

Commit b6c898e

Browse files
refactor: address comments.
1 parent 6cc7dd7 commit b6c898e

5 files changed

Lines changed: 64 additions & 48 deletions

File tree

docs/next-steps.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,3 +61,10 @@ Focused follow-up work for `@knighted/develop`.
6161
- `const App = () => <Button />`
6262
- Suggested implementation prompt:
6363
- "Document the current implicit App behavior in @knighted/develop for auto-render mode using a compact behavior matrix and concrete component-editor snippets. Clearly distinguish supported implicit wrapping from cases that intentionally require an explicit App (such as top-level JSX mixed with imports/declarations). Keep docs concise, aligned with current runtime behavior, and include at least one positive and one explicit-error example."
64+
65+
7. **Evaluate GitHub file upsert request strategy (metadata-first vs optimistic PUT)**
66+
- Revisit the current metadata-first `upsertRepositoryFile` approach and compare it against an optimistic PUT + targeted retry-on-missing-sha flow.
67+
- Measure tradeoffs for latency, GitHub API request count/rate-limit impact, and browser-console signal quality during common PR flows.
68+
- If beneficial, introduce a configurable/hybrid strategy (for example, optimistic default with metadata fallback) without regressing current reliability.
69+
- Suggested implementation prompt:
70+
- "Evaluate and optionally optimize @knighted/develop GitHub file upsert behavior. Compare metadata-first preflight GET+PUT against optimistic PUT with retry-on-missing-sha for existing files. Keep current reliability guarantees and avoid reintroducing noisy false-positive failures. If implementing a hybrid/configurable strategy, keep defaults conservative, update docs, and validate with npm run lint plus targeted Playwright PR drawer flows."

playwright/github-pr-drawer.spec.ts

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -171,8 +171,12 @@ test('Open PR drawer confirms and submits component/styles filepaths', async ({
171171
await expect(page.getByLabel('PR description')).toHaveValue(
172172
'Generated from editor content in @knighted/develop.',
173173
)
174-
await expect(page.locator('#github-pr-toggle')).toContainText('Push')
175-
await expect(page.locator('#github-pr-context-close')).toBeVisible()
174+
await expect(
175+
page.getByRole('button', { name: 'Push commit to active pull request branch' }),
176+
).toBeVisible()
177+
await expect(
178+
page.getByRole('button', { name: 'Close active pull request context' }),
179+
).toBeVisible()
176180
})
177181

178182
test('Open PR drawer base dropdown updates from mocked repo branches', async ({
@@ -504,8 +508,12 @@ test('Active PR context updates controls and can be closed from AI controls', as
504508

505509
await connectByotWithSingleRepo(page)
506510

507-
await expect(page.locator('#github-pr-toggle')).toContainText('Push')
508-
await expect(page.locator('#github-pr-context-close')).toBeVisible()
511+
await expect(
512+
page.getByRole('button', { name: 'Push commit to active pull request branch' }),
513+
).toBeVisible()
514+
await expect(
515+
page.getByRole('button', { name: 'Close active pull request context' }),
516+
).toBeVisible()
509517

510518
await page.getByRole('button', { name: 'Close active pull request context' }).click()
511519

@@ -514,8 +522,10 @@ test('Active PR context updates controls and can be closed from AI controls', as
514522
await expect(page.getByText('PR: develop/pr/2')).toBeVisible()
515523
await dialog.getByRole('button', { name: 'Close context' }).click()
516524

517-
await expect(page.locator('#github-pr-toggle')).toContainText('Open PR')
518-
await expect(page.locator('#github-pr-context-close')).toBeHidden()
525+
await expect(page.getByRole('button', { name: 'Open pull request' })).toBeVisible()
526+
await expect(
527+
page.getByRole('button', { name: 'Close active pull request context' }),
528+
).toBeHidden()
519529

520530
const storedValue = await page.evaluate(() =>
521531
localStorage.getItem('knighted:develop:github-pr-config:knightedcodemonkey/develop'),
@@ -588,8 +598,10 @@ test('Active PR context is disabled on load when pull request is closed', async
588598

589599
await connectByotWithSingleRepo(page)
590600

591-
await expect(page.locator('#github-pr-toggle')).toContainText('Open PR')
592-
await expect(page.locator('#github-pr-context-close')).toBeHidden()
601+
await expect(page.getByRole('button', { name: 'Open pull request' })).toBeVisible()
602+
await expect(
603+
page.getByRole('button', { name: 'Close active pull request context' }),
604+
).toBeHidden()
593605
await expect(
594606
page.getByRole('status', { name: 'Open pull request status', includeHidden: true }),
595607
).toContainText('Saved pull request context is not open on GitHub.')
@@ -945,10 +957,12 @@ test('Reloaded active PR context from URL metadata keeps Push mode and status re
945957

946958
await connectByotWithSingleRepo(page)
947959

948-
await expect(page.locator('#github-pr-toggle')).toContainText('Push')
960+
await expect(
961+
page.getByRole('button', { name: 'Push commit to active pull request branch' }),
962+
).toBeVisible()
949963
await ensureOpenPrDrawerOpen(page)
950964
await expect(page.getByRole('button', { name: 'Push commit' }).last()).toBeVisible()
951-
await expect(page.locator('#github-pr-head-branch')).toHaveValue('develop/open-pr-test')
965+
await expect(page.getByLabel('Head')).toHaveValue('develop/open-pr-test')
952966

953967
await setComponentEditorSource(page, 'const commitMarker = 1')
954968
await setStylesEditorSource(page, '.commit-marker { color: red; }')
@@ -1085,7 +1099,7 @@ test('Reloaded active PR context syncs editor content from GitHub branch', async
10851099
})
10861100

10871101
await connectByotWithSingleRepo(page)
1088-
await expect(page.locator('#render-mode')).toHaveValue('react')
1102+
await expect(page.getByLabel('Render mode')).toHaveValue('react')
10891103

10901104
await expect
10911105
.poll(async () =>

playwright/helpers/app-test-helpers.ts

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -150,15 +150,19 @@ export const ensureAiChatDrawerOpen = async (page: Page) => {
150150
}
151151

152152
export const ensureOpenPrDrawerOpen = async (page: Page) => {
153-
const toggle = page.locator('#github-pr-toggle')
153+
const toggle = page.getByRole('button', {
154+
name: /Open pull request|Push commit to active pull request branch/,
155+
})
154156
await expect(toggle).toBeEnabled({ timeout: 60_000 })
155157
const isExpanded = await toggle.getAttribute('aria-expanded')
156158

157159
if (isExpanded !== 'true') {
158160
await toggle.click()
159161
}
160162

161-
await expect(page.locator('#github-pr-drawer')).toBeVisible()
163+
await expect(
164+
page.getByRole('complementary', { name: /Open Pull Request|Push Commit/ }),
165+
).toBeVisible()
162166
}
163167

164168
export const mockRepositoryBranches = async (
@@ -210,20 +214,15 @@ export const connectByotWithSingleRepo = async (page: Page) => {
210214
.fill('github_pat_fake_chat_1234567890')
211215
await page.getByRole('button', { name: 'Add GitHub token' }).click()
212216

213-
await expect
214-
.poll(() =>
215-
page.evaluate(() => {
216-
const repoSelect = document.getElementById('github-pr-repo-select')
217-
if (!(repoSelect instanceof HTMLSelectElement)) {
218-
return false
219-
}
220-
221-
return !repoSelect.disabled && repoSelect.value === 'knightedcodemonkey/develop'
222-
}),
223-
)
224-
.toBe(true)
225-
226-
await expect(page.locator('#github-pr-toggle')).toBeVisible()
217+
const repoSelect = page.getByLabel('Pull request repository')
218+
await expect(repoSelect).toBeEnabled({ timeout: 60_000 })
219+
await expect(repoSelect).toHaveValue('knightedcodemonkey/develop')
220+
221+
await expect(
222+
page.getByRole('button', {
223+
name: /Open pull request|Push commit to active pull request branch/,
224+
}),
225+
).toBeVisible()
227226
}
228227

229228
export const expectCollapseButtonState = async (

src/app.js

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -871,25 +871,26 @@ githubPrContextClose?.addEventListener('click', () => {
871871
const referenceLine = activePrReference ? `PR: ${activePrReference}\n` : ''
872872

873873
confirmAction({
874-
title: 'Close active pull request context?',
875-
copy: `${referenceLine}PR title: ${githubAiContextState.activePrContext.prTitle}\nHead branch: ${githubAiContextState.activePrContext.headBranch}\n\nThis clears the active pull request context for the selected repository.`,
876-
confirmButtonText: 'Close context',
877-
fallbackConfirmText: 'Close active pull request context for the selected repository?',
874+
title: 'Close pull request on GitHub?',
875+
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.`,
876+
confirmButtonText: 'Close PR on GitHub',
877+
fallbackConfirmText:
878+
'Close this pull request on GitHub and clear the active context for the selected repository?',
878879
onConfirm: () => {
879880
void prDrawerController
880881
.closeActivePullRequestOnGitHub()
881882
.then(result => {
882883
const reference = result?.reference
883884
setStatus(
884885
reference
885-
? `Closed active pull request context (${reference}).`
886-
: 'Closed active pull request context.',
886+
? `Closed pull request on GitHub and cleared active context (${reference}).`
887+
: 'Closed pull request on GitHub and cleared active context.',
887888
'neutral',
888889
)
889890
showAppToast(
890891
reference
891-
? `Closed active pull request context (${reference}).`
892-
: 'Closed active pull request context.',
892+
? `Closed pull request on GitHub and cleared active context (${reference}).`
893+
: 'Closed pull request on GitHub and cleared active context.',
893894
)
894895
})
895896
.catch(error => {

src/modules/render-runtime.js

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -966,20 +966,15 @@ export const createRenderRuntimeController = ({
966966
}
967967
}
968968

969-
const processQueuedRenderPasses = async () => {
970-
if (!rerenderRequested) {
971-
return
972-
}
973-
974-
rerenderRequested = false
975-
await runRenderPass()
976-
await processQueuedRenderPasses()
977-
}
978-
979969
try {
980-
rerenderRequested = false
981-
await runRenderPass()
982-
await processQueuedRenderPasses()
970+
rerenderRequested = true
971+
972+
while (rerenderRequested) {
973+
rerenderRequested = false
974+
// Intentionally sequential to drain queued renders without recursion.
975+
// eslint-disable-next-line no-await-in-loop
976+
await runRenderPass()
977+
}
983978
} finally {
984979
renderInFlight = false
985980
}

0 commit comments

Comments
 (0)