feature(web): unify crawler detail/new into split-pane editor (TES-28)#25
Conversation
EditorWrapper with flex+min-height caused Monaco internal height: 100% to fail resolving (CSS spec — % requires parent explicit height). Use fixed height: 500px on wrapper and explicit height="100%" on Editor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
op read failed with ENOENT because the actual vault item name is 'Service Account - Audio Underview', not 'Service Account Auth Token - Audio Underview'. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthrough크롤러 생성과 편집 기능을 단일 Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Editor as CrawlerEditorPage
participant Hook as useCrawlerEditor
participant Form as useCrawlerForm
participant API as API
participant Dialog as DiscardDialog
User->>Editor: 페이지 진입 (생성 또는 편집)
Editor->>Hook: useCrawlerEditor(id) 호출
alt 편집 모드 (id 존재)
Hook->>API: 크롤러 데이터 로드
API-->>Hook: 크롤러 데이터 반환
Hook->>Form: deriveFormState로 폼 초기화
else 생성 모드 (id 없음)
Hook->>Form: BLANK_FORM으로 초기화
end
Hook-->>Editor: 편집 상태, 폼 필드, 저장 함수 제공
Editor-->>User: 뷰 모드로 렌더링
User->>Editor: 편집 버튼 클릭
Editor->>Hook: enterEdit() 호출
Hook-->>Editor: 편집 모드 활성화
Editor-->>User: 편집 가능 UI 렌더링
User->>Editor: 폼 필드 수정
Editor->>Form: 폼 상태 업데이트
Form->>Hook: isDirty 계산
Hook->>Hook: useBeforeUnload(isDirty) 활성화
alt 저장 클릭
User->>Editor: 저장 버튼 클릭
Editor->>Hook: save() 호출
Hook->>Form: 스키마 유효성 검사
Hook->>API: createCrawler/updateCrawler 호출
API-->>Hook: 성공 응답
Hook-->>Editor: 저장 완료
Editor-->>User: 성공 토스트 표시
else 취소 클릭 (미저장 변경 있음)
User->>Editor: 취소 버튼 클릭
Editor->>Dialog: DiscardDialog 열기
Dialog-->>User: 확인 다이얼로그 표시
User->>Dialog: 폐기 확인
Dialog->>Hook: discard() 호출
Hook->>Form: 폼 리셋
Hook-->>Editor: 뷰 모드로 전환
Editor-->>User: 변경사항 폐기됨
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Replaces CrawlerDetailPage and CrawlerNewPage with a single CrawlerEditorPage that handles view/edit/create on /crawlers/:id (id='new' = create mode), following the Cloudflare/Supabase split-pane editor pattern. - /crawlers/:id route renders CrawlerEditorPage in all three modes - Split-pane layout: form left, test runner right; toggleable in view (localStorage), always on in edit/create - Hook split: use-crawler-editor (orchestrator) over use-crawler-form, use-editor-mode, use-before-unload, plus pure crawler-form-helpers - Custom DiscardDialog (Radix alertdialog) replaces every window.confirm in the codebase - FieldLabel + htmlFor on name/url_pattern, aria-labelledby on schemas, aria-disabled + aria-describedby with computed reason on Save - aria-live count on Logs button, FormColumn no longer scroll-traps at zoom (WCAG 1.4.10) - Keyboard shortcuts (Cmd+S/E/Enter) via React 19.2 useEffectEvent; Cmd+E skipped in form fields - inFlightRef guards save() against double-submit; 16/16 tests pass Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
gignac-cha
force-pushed
the
feature/crawler-detail-view-edit-mode
branch
from
efef358 to
98c1e33
Compare
|
@coderabbitai review 다음 영역을 중점적으로 검토 부탁드립니다. 아키텍처 / hook 분리
React 19.2 패턴
접근성
UX / 동작
견고성
이번 PR scope 외 (블로커로 표시 X)
|
🧩 Analysis chain🏁 Script executed: Length of output: 56544
🏗️ 아키텍처 / Hook 분리결론: 깔끔합니다. 누수 추상화나 state 중복 없음.
소소한 관찰: 🔄 Render-phase Seeding결론: StrictMode 가드 충분합니다. if (!isCreateMode && crawler && seededForCrawlerID !== crawler.id) {
setSeededForCrawlerID(crawler.id);
formState.resetFromCrawler(crawler);
modeState.exitEdit();
}React Strict Mode의 double-invoke 시나리오:
단, 단일 render에서 여러 ⚛️ React 19.2 패턴
|
| 영역 | 상태 | 비고 |
|---|---|---|
| Hook 책임 경계 | ✅ 깔끔 | disabledReason 헬퍼 추출 여지 (minor) |
| Render-phase seeding | ✅ StrictMode 안전 | 추가 렌더 1회 (React 공식 패턴) |
id === 'new' sentinel |
✅ 실용적 | UUID 충돌 불가, 관용적 패턴 |
useEffectEvent |
✅ 올바른 사용 | React 19 stable API |
inFlightRef 가드 |
✅ 모든 경로 안전 | |
aria-disabled |
✅ disabled 완전 대체 |
SR 경험 오히려 향상 |
FieldLabel / MetaLabel |
✅ 의도된 설계 | |
DiscardDialog |
✅ WCAG 충족 | |
aria-live in <button> |
버튼 외부로 이동 권장 | |
pendingDiscardActionRef |
✅ 이중 실행 없음 | |
markSaved race |
✅ 커버됨 | |
| 실행 로그 미초기화 | 크롤러 전환 시 |
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
applications/web/sources/pages/CrawlerEditorPage.test.tsx (1)
82-146: 🧹 Nitpick | 🔵 Trivial테스트 간
localStorage정리를beforeEach로 일원화 권장
TEST_PANEL_STORAGE_KEY를 일부 테스트(예: Line 84, 108, 126)에서만 제거하고 나머지 테스트(특히 dirty/Cancel/Save 관련 다수와 create-flow 두 테스트)에서는 제거하지 않습니다. 현재는 edit/create 모드가 테스트 패널을 강제로 켜기 때문에 단언이 모두 통과하지만, 향후 뷰 모드 기본값이나 저장 키 의미가 바뀌면 실행 순서에 따라 조용히 flaky해질 수 있습니다.♻️ 제안 수정
describe('CrawlerEditorPage — view/edit flows', () => { + beforeEach(() => { + window.localStorage.removeItem(TEST_PANEL_STORAGE_KEY); + }); + test('renders crawler details read-only by default with test panel hidden', async () => { - window.localStorage.removeItem(TEST_PANEL_STORAGE_KEY); - worker.use(create-flow
describe에도 동일한beforeEach를 추가하면 전체 스위트가 테스트 순서 독립이 됩니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/web/sources/pages/CrawlerEditorPage.test.tsx` around lines 82 - 146, Add a beforeEach that clears window.localStorage.removeItem(TEST_PANEL_STORAGE_KEY) inside the describe block for "CrawlerEditorPage — view/edit flows" (and mirror the same beforeEach in the separate "create-flow" describe) so each test starts with a clean TEST_PANEL_STORAGE_KEY state; this replaces the scattered manual removals in individual tests and ensures tests referencing TEST_PANEL_STORAGE_KEY are order-independent.applications/web/sources/components/crawlers/CodeEditorPanel.tsx (1)
41-46: 🧹 Nitpick | 🔵 Trivial고정 500px 높이가 split-pane 환경에서 확장성/반응성을 떨어뜨립니다.
Monaco가 flex/
min-height: 0컨테이너에서 레이아웃을 잡지 못하는 이슈를 고정 높이로 우회한 것으로 보입니다. 다만 이 패널은 새 split-pane 에디터의 좌측 패널에서 사용되므로,
- 뷰포트가 클 때: 남는 세로 공간을 활용하지 못하고 500px에 고정됩니다.
- 뷰포트가 작을 때(모바일/태블릿): 500px이 페이지 전체 세로 스크롤을 유발할 수 있습니다.
min-height로 바꾸고 부모가 높이를 제공하는 구조를 유지하면 반응성을 회복할 수 있습니다.♻️ 제안 수정
const EditorWrapper = styled.div` - height: 500px; + min-height: 500px; + flex: 1; border: 1px solid var(--border-subtle); border-radius: 8px; overflow: hidden; `;부모
Container가flex: 1; min-height: 0;를 유지하고 있으므로Editor의height="100%"+automaticLayout: true조합으로 Monaco가 올바르게 리사이즈됩니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/web/sources/components/crawlers/CodeEditorPanel.tsx` around lines 41 - 46, The EditorWrapper currently uses a fixed height (height: 500px) which breaks responsiveness; change EditorWrapper (styled.div) to avoid a fixed height (use min-height: 0 and/or height: 100% instead) so the parent Container (ensure it has flex: 1; min-height: 0) can provide sizing, and make sure the Editor component is rendered with height="100%" and Monaco's automaticLayout: true so the editor resizes correctly in split-pane layouts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@applications/web/sources/hooks/use-before-unload.ts`:
- Around line 6-8: The beforeunload handler currently only calls
event.preventDefault(), which won't trigger the confirmation dialog in some
browsers; update the handler function (the const handler in
use-before-unload.ts) to set both event.preventDefault() AND assign
event.returnValue = '' (or a descriptive string) to ensure compatibility with
Chrome <119 and Safari, keeping the same cleanup/unsubscribe logic.
In `@applications/web/sources/hooks/use-crawler-editor.ts`:
- Around line 179-183: markSaved currently doesn't indicate whether the
submitted changes were actually applied, so calling modeState.exitEdit()
unconditionally can hide newly made edits; change markSaved to return a boolean
(true if it accepted/applied the submittedForm vs next identity check) and in
this block capture that result from markSaved(submittedForm, next) and only call
modeState.exitEdit() when it returns true; this involves updating the markSaved
signature and its callers and keeping the existing calls to
deriveFormState(updated) and updateCrawler(payload) as-is.
- Around line 22-34: When isCreateMode changes the form and editor mode must be
reset; add a useEffect in the useCrawlerForm hook that listens to [isCreateMode]
and calls the hook's reset logic (e.g., setForm(...)/setPristine(...) or the
hook's existing reset method) to clear previous crawler data, and add a
useEffect in useEditorMode that listens to [isCreateMode] and sets edit mode off
(e.g., setIsEditMode(false)). This ensures useCrawlerForm and useEditorMode
react to isCreateMode changes instead of only initializing once.
In `@applications/web/sources/pages/CrawlerEditorPage.tsx`:
- Around line 479-510: When the active crawler id changes, clear per-crawler UI
and runner state by adding an effect that watches the crawler id and on change
calls setTestURL(''), setExecutionLogs([]), setIsLogOverlayOpen(false) and
resets the test panel visibility (via setViewShowTest) as appropriate, and also
invoke the code runner reset method returned from useCrawlerCodeRunner (call its
reset() function) so runTest/runStatus/runResult/runError are cleared; implement
this effect near the existing state/hooks that reference testURL, executionLogs,
viewShowTest, handleLog and useCrawlerCodeRunner.
- Around line 787-790: The live region for log count should be moved out of the
interactive GhostButton to avoid unreliable AT notifications; keep the button
label showing the count (e.g., "Logs (N)") but remove aria-live/aria-atomic from
inside GhostButton, and add a separate visually-hidden element (e.g., a span
with aria-live="polite" aria-atomic="true" and hidden/status-only styling) that
updates with executionLogs.length; update the GhostButton usage (onClick={() =>
setIsLogOverlayOpen(true)}) to no longer contain aria-live attributes and ensure
the new hidden live-region element is updated whenever executionLogs changes.
In `@tools/environment-generator/sources/setup.ts`:
- Line 8: Update the README.md to document the exact 1Password item name and
required field so the environment generator can find it: state that developers
must create a 1Password Personal item named "Service Account - Audio Underview"
and include a field named "credential" (matching the op://Personal/Service
Account - Audio Underview/credential reference in setup.ts); add a short note
about where this value is used by the setup script and an example of the
item/field pair to prevent lookup failures.
---
Outside diff comments:
In `@applications/web/sources/components/crawlers/CodeEditorPanel.tsx`:
- Around line 41-46: The EditorWrapper currently uses a fixed height (height:
500px) which breaks responsiveness; change EditorWrapper (styled.div) to avoid a
fixed height (use min-height: 0 and/or height: 100% instead) so the parent
Container (ensure it has flex: 1; min-height: 0) can provide sizing, and make
sure the Editor component is rendered with height="100%" and Monaco's
automaticLayout: true so the editor resizes correctly in split-pane layouts.
In `@applications/web/sources/pages/CrawlerEditorPage.test.tsx`:
- Around line 82-146: Add a beforeEach that clears
window.localStorage.removeItem(TEST_PANEL_STORAGE_KEY) inside the describe block
for "CrawlerEditorPage — view/edit flows" (and mirror the same beforeEach in the
separate "create-flow" describe) so each test starts with a clean
TEST_PANEL_STORAGE_KEY state; this replaces the scattered manual removals in
individual tests and ensures tests referencing TEST_PANEL_STORAGE_KEY are
order-independent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e4199a9b-8430-441a-a57f-e705b0eb89e6
📒 Files selected for processing (15)
applications/web/sources/Application.tsxapplications/web/sources/components/DiscardDialog.tsxapplications/web/sources/components/crawlers/CodeEditorPanel.tsxapplications/web/sources/components/crawlers/CrawlerSubmissionDialog.test.tsxapplications/web/sources/components/crawlers/CrawlerSubmissionDialog.tsxapplications/web/sources/hooks/crawler-form-helpers.tsapplications/web/sources/hooks/use-before-unload.tsapplications/web/sources/hooks/use-crawler-editor.tsapplications/web/sources/hooks/use-crawler-form.tsapplications/web/sources/hooks/use-editor-mode.tsapplications/web/sources/pages/CrawlerDetailPage.tsxapplications/web/sources/pages/CrawlerEditorPage.test.tsxapplications/web/sources/pages/CrawlerEditorPage.tsxapplications/web/sources/pages/CrawlerNewPage.tsxtools/environment-generator/sources/setup.ts
💤 Files with no reviewable changes (4)
- applications/web/sources/components/crawlers/CrawlerSubmissionDialog.test.tsx
- applications/web/sources/components/crawlers/CrawlerSubmissionDialog.tsx
- applications/web/sources/pages/CrawlerDetailPage.tsx
- applications/web/sources/pages/CrawlerNewPage.tsx
| const handler = (event: BeforeUnloadEvent) => { | ||
| event.preventDefault(); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Chrome, Safari and Firefox in 2025 require setting event.returnValue on beforeunload, or is preventDefault() alone sufficient across major browsers?
💡 Result:
No, preventDefault alone is not sufficient across major browsers like Chrome, Safari, and Firefox in 2025. Both preventDefault and setting event.returnValue are recommended for full compatibility. Modern best practice (post-Chrome 119, current versions of all browsers) is to call event.preventDefault to trigger the confirmation dialog. This is fully supported in Chrome 119+, Firefox 6+, Safari 11+. However, MDN explicitly advises also setting event.returnValue = true for legacy support, noting "e.g. Chrome/Edge < 119". While pre-119 versions are outdated by 2025, some users or enterprise environments may still use them. CanIUse confirms returnValue support is widespread, but preventDefault activation became standard later. The recommended code from MDN (updated 2025): const beforeUnloadHandler = (event) => { event.preventDefault; // Recommended for modern browsers event.returnValue = true; // Legacy support }; Sources confirm this dual approach ensures the dialog triggers reliably across Chrome, Safari, and Firefox, including any lingering legacy cases.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/BeforeUnloadEvent/returnValue
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event
- 3: https://caniuse.com/mdn-api_window_beforeunload_event_preventdefault_activation
- 4: https://caniuse.com/mdn-api_beforeunloadevent_returnvalue
레거시 브라우저 호환성: event.returnValue 설정 필수
preventDefault() 만으로는 Chrome 119 이전 버전과 일부 Safari에서 beforeunload 확인 프롬프트가 나타나지 않습니다. 저장되지 않은 변경사항 보호라는 이 훅의 목적을 확실히 하려면 두 방식을 모두 적용해야 합니다.
제안 수정
const handler = (event: BeforeUnloadEvent) => {
event.preventDefault();
+ event.returnValue = '';
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@applications/web/sources/hooks/use-before-unload.ts` around lines 6 - 8, The
beforeunload handler currently only calls event.preventDefault(), which won't
trigger the confirmation dialog in some browsers; update the handler function
(the const handler in use-before-unload.ts) to set both event.preventDefault()
AND assign event.returnValue = '' (or a descriptive string) to ensure
compatibility with Chrome <119 and Safari, keeping the same cleanup/unsubscribe
logic.
| const formState = useCrawlerForm(isCreateMode); | ||
| const modeState = useEditorMode(isCreateMode); | ||
|
|
||
| const [seededForCrawlerID, setSeededForCrawlerID] = useState<string | undefined>(undefined); | ||
| const [discardDialogOpen, setDiscardDialogOpen] = useState(false); | ||
| const pendingDiscardActionRef = useRef<(() => void) | null>(null); | ||
| const inFlightRef = useRef(false); | ||
|
|
||
| if (!isCreateMode && crawler && seededForCrawlerID !== crawler.id) { | ||
| setSeededForCrawlerID(crawler.id); | ||
| formState.resetFromCrawler(crawler); | ||
| modeState.exitEdit(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -i 'use-(crawler-form|editor-mode)\.ts$' --exec sh -c '
printf "\n=== %s ===\n" "$1"
sed -n "1,240p" "$1"
' sh {}Repository: gignac-cha/audio-underview
Length of output: 3012
isCreateMode 변경 시 폼 및 모드 상태를 초기화하는 effect 훅 추가 필요
useCrawlerForm과 useEditorMode 모두 isCreateMode 기반으로 초기 상태만 설정하고, 해당 prop 변경에 반응하는 effect 훅이 없습니다. 같은 컴포넌트에서 기존 크롤러 → /crawlers/new 이동 시 두 훅의 상태가 유지되어, 이전 폼 데이터와 edit 모드가 create 화면에 남게 됩니다.
useCrawlerForm에 useEffect(() => { setForm(...); setPristine(...); }, [isCreateMode])를 추가하고, useEditorMode에 useEffect(() => { setIsEditMode(false); }, [isCreateMode])를 추가하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@applications/web/sources/hooks/use-crawler-editor.ts` around lines 22 - 34,
When isCreateMode changes the form and editor mode must be reset; add a
useEffect in the useCrawlerForm hook that listens to [isCreateMode] and calls
the hook's reset logic (e.g., setForm(...)/setPristine(...) or the hook's
existing reset method) to clear previous crawler data, and add a useEffect in
useEditorMode that listens to [isCreateMode] and sets edit mode off (e.g.,
setIsEditMode(false)). This ensures useCrawlerForm and useEditorMode react to
isCreateMode changes instead of only initializing once.
| const submittedForm = formState.form; | ||
| const updated = await updateCrawler(payload); | ||
| const next = deriveFormState(updated); | ||
| formState.markSaved(submittedForm, next); | ||
| modeState.exitEdit(); |
There was a problem hiding this comment.
저장 중 추가 편집이 있으면 edit 모드를 유지하세요.
markSaved가 제출 당시 form이 아닌 최신 form을 보존하더라도, 바로 exitEdit()를 호출하면 보존된 미저장 변경사항이 view 모드에 숨겨질 수 있습니다. markSaved가 실제 적용 여부를 반환하게 하고, 적용된 경우에만 view 모드로 전환하세요.
🐛 제안 수정
const submittedForm = formState.form;
const updated = await updateCrawler(payload);
const next = deriveFormState(updated);
- formState.markSaved(submittedForm, next);
- modeState.exitEdit();
+ const didMarkSubmittedFormAsSaved = formState.markSaved(submittedForm, next);
+ if (didMarkSubmittedFormAsSaved) {
+ modeState.exitEdit();
+ }
showToast('Saved', `Crawler "${trimmedName}" has been updated.`, 'success');markSaved가 현재 void를 반환한다면, identity check 성공 여부를 boolean으로 반환하도록 함께 변경해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@applications/web/sources/hooks/use-crawler-editor.ts` around lines 179 - 183,
markSaved currently doesn't indicate whether the submitted changes were actually
applied, so calling modeState.exitEdit() unconditionally can hide newly made
edits; change markSaved to return a boolean (true if it accepted/applied the
submittedForm vs next identity check) and in this block capture that result from
markSaved(submittedForm, next) and only call modeState.exitEdit() when it
returns true; this involves updating the markSaved signature and its callers and
keeping the existing calls to deriveFormState(updated) and
updateCrawler(payload) as-is.
| const [testURL, setTestURL] = useState(''); | ||
| const [executionLogs, setExecutionLogs] = useState<LogEntry[]>([]); | ||
| const [isLogOverlayOpen, setIsLogOverlayOpen] = useState(false); | ||
|
|
||
| const [viewShowTest, setViewShowTest] = useState<boolean>(() => { | ||
| try { | ||
| return window.localStorage.getItem(TEST_PANEL_STORAGE_KEY) === 'open'; | ||
| } catch { | ||
| return false; | ||
| } | ||
| }); | ||
|
|
||
| const showTest = mode === 'view' ? viewShowTest : true; | ||
|
|
||
| const handleToggleTest = () => { | ||
| if (mode !== 'view') return; | ||
| setViewShowTest((previous) => { | ||
| const next = !previous; | ||
| try { | ||
| window.localStorage.setItem(TEST_PANEL_STORAGE_KEY, next ? 'open' : 'closed'); | ||
| } catch { | ||
| /* ignore */ | ||
| } | ||
| return next; | ||
| }); | ||
| }; | ||
|
|
||
| const handleLog = useCallback((entry: LogEntry) => { | ||
| setExecutionLogs((prev) => [...prev, entry]); | ||
| }, []); | ||
|
|
||
| const { runTest, status: runStatus, result: runResult, error: runError } = useCrawlerCodeRunner({ onLog: handleLog }); |
There was a problem hiding this comment.
크롤러 전환 시 테스트 실행 상태를 초기화하세요.
같은 CrawlerEditorPage 인스턴스에서 id만 바뀌면 이전 크롤러의 로그, 결과, 에러가 새 크롤러 화면에 남을 수 있습니다. useCrawlerCodeRunner.reset()까지 함께 호출해 결과 패널도 비워 주세요.
🐛 제안 수정
- const { runTest, status: runStatus, result: runResult, error: runError } = useCrawlerCodeRunner({ onLog: handleLog });
+ const {
+ runTest,
+ status: runStatus,
+ result: runResult,
+ error: runError,
+ reset: resetRunState,
+ } = useCrawlerCodeRunner({ onLog: handleLog });
const isRunning = runStatus === 'running';
+
+ useEffect(() => {
+ setExecutionLogs([]);
+ setIsLogOverlayOpen(false);
+ resetRunState();
+ }, [id, resetRunState]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [testURL, setTestURL] = useState(''); | |
| const [executionLogs, setExecutionLogs] = useState<LogEntry[]>([]); | |
| const [isLogOverlayOpen, setIsLogOverlayOpen] = useState(false); | |
| const [viewShowTest, setViewShowTest] = useState<boolean>(() => { | |
| try { | |
| return window.localStorage.getItem(TEST_PANEL_STORAGE_KEY) === 'open'; | |
| } catch { | |
| return false; | |
| } | |
| }); | |
| const showTest = mode === 'view' ? viewShowTest : true; | |
| const handleToggleTest = () => { | |
| if (mode !== 'view') return; | |
| setViewShowTest((previous) => { | |
| const next = !previous; | |
| try { | |
| window.localStorage.setItem(TEST_PANEL_STORAGE_KEY, next ? 'open' : 'closed'); | |
| } catch { | |
| /* ignore */ | |
| } | |
| return next; | |
| }); | |
| }; | |
| const handleLog = useCallback((entry: LogEntry) => { | |
| setExecutionLogs((prev) => [...prev, entry]); | |
| }, []); | |
| const { runTest, status: runStatus, result: runResult, error: runError } = useCrawlerCodeRunner({ onLog: handleLog }); | |
| const [testURL, setTestURL] = useState(''); | |
| const [executionLogs, setExecutionLogs] = useState<LogEntry[]>([]); | |
| const [isLogOverlayOpen, setIsLogOverlayOpen] = useState(false); | |
| const [viewShowTest, setViewShowTest] = useState<boolean>(() => { | |
| try { | |
| return window.localStorage.getItem(TEST_PANEL_STORAGE_KEY) === 'open'; | |
| } catch { | |
| return false; | |
| } | |
| }); | |
| const showTest = mode === 'view' ? viewShowTest : true; | |
| const handleToggleTest = () => { | |
| if (mode !== 'view') return; | |
| setViewShowTest((previous) => { | |
| const next = !previous; | |
| try { | |
| window.localStorage.setItem(TEST_PANEL_STORAGE_KEY, next ? 'open' : 'closed'); | |
| } catch { | |
| /* ignore */ | |
| } | |
| return next; | |
| }); | |
| }; | |
| const handleLog = useCallback((entry: LogEntry) => { | |
| setExecutionLogs((prev) => [...prev, entry]); | |
| }, []); | |
| const { | |
| runTest, | |
| status: runStatus, | |
| result: runResult, | |
| error: runError, | |
| reset: resetRunState, | |
| } = useCrawlerCodeRunner({ onLog: handleLog }); | |
| useEffect(() => { | |
| setExecutionLogs([]); | |
| setIsLogOverlayOpen(false); | |
| resetRunState(); | |
| }, [id, resetRunState]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@applications/web/sources/pages/CrawlerEditorPage.tsx` around lines 479 - 510,
When the active crawler id changes, clear per-crawler UI and runner state by
adding an effect that watches the crawler id and on change calls setTestURL(''),
setExecutionLogs([]), setIsLogOverlayOpen(false) and resets the test panel
visibility (via setViewShowTest) as appropriate, and also invoke the code runner
reset method returned from useCrawlerCodeRunner (call its reset() function) so
runTest/runStatus/runResult/runError are cleared; implement this effect near the
existing state/hooks that reference testURL, executionLogs, viewShowTest,
handleLog and useCrawlerCodeRunner.
| <GhostButton onClick={() => setIsLogOverlayOpen(true)} aria-label="Open logs"> | ||
| Logs ( | ||
| <span aria-live="polite" aria-atomic="true">{executionLogs.length}</span> | ||
| ) |
There was a problem hiding this comment.
로그 카운트 live region을 버튼 밖으로 분리하세요.
aria-live가 버튼 내부에 있으면 보조기술별 알림 동작이 불안정할 수 있습니다. 버튼 이름에는 카운트를 포함하고, 별도 hidden status를 두는 편이 안전합니다.
♿ 제안 수정
- <GhostButton onClick={() => setIsLogOverlayOpen(true)} aria-label="Open logs">
+ <GhostButton onClick={() => setIsLogOverlayOpen(true)} aria-label={`Open logs (${executionLogs.length})`}>
Logs (
- <span aria-live="polite" aria-atomic="true">{executionLogs.length}</span>
+ {executionLogs.length}
)
</GhostButton>
+ <VisuallyHidden role="status" aria-live="polite" aria-atomic="true">
+ {executionLogs.length} log entries
+ </VisuallyHidden>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@applications/web/sources/pages/CrawlerEditorPage.tsx` around lines 787 - 790,
The live region for log count should be moved out of the interactive GhostButton
to avoid unreliable AT notifications; keep the button label showing the count
(e.g., "Logs (N)") but remove aria-live/aria-atomic from inside GhostButton, and
add a separate visually-hidden element (e.g., a span with aria-live="polite"
aria-atomic="true" and hidden/status-only styling) that updates with
executionLogs.length; update the GhostButton usage (onClick={() =>
setIsLogOverlayOpen(true)}) to no longer contain aria-live attributes and ensure
the new hidden live-region element is updated whenever executionLogs changes.
| const onePasswordReferencePath = | ||
| process.env.OP_SERVICE_ACCOUNT_REFERENCE ?? | ||
| 'op://Personal/Service Account Auth Token - Audio Underview/credential'; | ||
| 'op://Personal/Service Account - Audio Underview/credential'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if README.md documents the exact 1Password item name and search for any references to the old item name
# Check if README mentions the specific item name
echo "=== Checking README for item name documentation ==="
rg -n "Service Account" tools/environment-generator/README.md
# Search for any other references to 1Password paths in the codebase
echo "=== Searching for other 1Password reference paths ==="
rg -n "op://Personal" --type ts --type md
# Check if there are any migration guides or changelogs mentioning this change
echo "=== Checking for migration documentation ==="
fd -e md | xargs rg -l "Service Account|1Password.*setup" 2>/dev/nullRepository: gignac-cha/audio-underview
Length of output: 465
🌐 Web query:
1Password CLI op:// reference path syntax and best practices for item naming
💡 Result:
The 1Password CLI uses op:// secret reference URIs with the syntax op://[vault]/[item]/[section]/[field], where each part can be a name or unique ID. Syntax details: - Vault: Name or ID of the vault (e.g., 'Management'). - Item: Name or ID of the item (e.g., 'PagerDuty'). - Section: Name or ID of the section (optional if no section; e.g., 'Admin'). - Field: Name or ID of the field (e.g., 'email'). - If a name contains unsupported characters (e.g., / in 'test/'), use its ID instead, obtained via op item get --format json. - Example: op://Management/PagerDuty/Admin/email refers to the 'email' field in the 'Admin' section of the 'PagerDuty' item in the 'Management' vault. Use commands like op read op://vault/item/section/field, op run, or op inject to resolve references. For item naming best practices: - No strict rules specified in official docs; choose descriptive titles for easy identification (e.g., via --title in op item create). - Use tags for organization (comma-separated with --tags; nestable with / like 'finance/insurance'). - Avoid names with special characters requiring IDs in references. - Organize with favorites, vaults, and tags rather than rigid naming. - When creating items (op item create), use descriptive titles, categories, and vaults; leverage JSON templates for complex items.
Citations:
- 1: https://developer.1password.com/docs/cli/secret-reference-syntax/
- 2: https://developer.1password.com/docs/cli/secret-references/
- 3: https://developer.1password.com/docs/cli/item-create/
- 4: https://developer.1password.com/docs/cli/best-practices/
- 5: https://support.1password.com/favorites-tags
README에 정확한 1Password 항목 이름을 문서화하세요.
현재 tools/environment-generator/README.md에는 1Password 설정이 필요하다는 것만 명시되어 있으며, 개발자가 생성해야 할 정확한 항목 이름 "Service Account - Audio Underview"가 문서화되어 있지 않습니다. 환경 설정 스크립트가 이 항목을 찾지 못할 경우 워크플로우가 실패하므로, README에 다음 정보를 추가하세요:
- 1Password Personal 계정에서 생성할 항목 이름:
Service Account - Audio Underview - 이 항목에 포함되어야 할 필드:
credential
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/environment-generator/sources/setup.ts` at line 8, Update the README.md
to document the exact 1Password item name and required field so the environment
generator can find it: state that developers must create a 1Password Personal
item named "Service Account - Audio Underview" and include a field named
"credential" (matching the op://Personal/Service Account - Audio
Underview/credential reference in setup.ts); add a short note about where this
value is used by the setup script and an example of the item/field pair to
prevent lookup failures.
- use-before-unload: set event.returnValue alongside preventDefault for Chrome <119 compatibility
- CrawlerEditorPage: wrap content in inner component with key={id ?? 'new'} so /crawlers/abc → /crawlers/new fully resets form, edit mode, executionLogs, and runner state via remount
- use-crawler-form: markSaved returns boolean (via formRef updated in useEffect) indicating whether the in-flight submission was actually applied
- use-crawler-editor: only call modeState.exitEdit() when markSaved applied — preserves user edits made during in-flight save instead of hiding them in view mode
- CrawlerEditorPage: move Logs aria-live count out of the button into a separate VisuallyHidden role=status region; button name now includes the count via aria-label
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai review 이전 리뷰 6개 actionable comments를 commit a6eca5a에서 처리했습니다. 다시 검토 부탁드립니다. 처리한 항목
검증: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 34653
✅ 수정 검증 결과1.
|
| 항목 | 상태 |
|---|---|
event.returnValue = '' |
✅ |
key={id ?? 'new'} unmount/remount |
✅ 제안보다 우수한 접근 |
markSaved boolean + formRef race guard |
✅ |
| runner state reset | ✅ (key wrapper 흡수) |
aria-live 버튼 외부 이동 |
✅ ARIA22 준수 |
| setup.ts README | ✅ (의도적 미변경, 근거 타당) |
16/16 테스트 통과 + lint clean 상태에서 모든 actionable item이 해결되었습니다. 승인합니다.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
1 participant
Summary
CrawlerDetailPage(view/edit) andCrawlerNewPage(create) into a singleCrawlerEditorPage. Route/crawlers/:idhandles all modes;id='new'triggers create./crawlers/newroute falls under:idmatching.use-crawler-editor(orchestrator) overuse-crawler-form+use-editor-mode+use-before-unload+ purecrawler-form-helpers. Old monolithic state hook removed.Closes TES-28.
Architecture
pages/CrawlerEditorPage.tsxhooks/use-crawler-editor.tshooks/use-crawler-form.tshooks/use-editor-mode.tshooks/use-before-unload.tshooks/crawler-form-helpers.tscomponents/DiscardDialog.tsxalertdialogreplacement forwindow.confirm.Removed:
CrawlerDetailPage.tsx,CrawlerNewPage.tsx,CrawlerSubmissionDialog.tsx(+test), olduse-crawler-editor-state.ts.Behavior changes
crawler-editor:test-panel-open.DiscardDialog(focus on Keep editing, Esc closes, focus returns to trigger). No more nativewindow.confirmanywhere in the codebase.aria-disabled+aria-describedbyreason ("No changes to save.", "Fix schema errors before saving.", "Name is required.", etc.) so users see why the button is unavailable.Cmd/Ctrl+S,+E,+Enter) implemented with React 19.2useEffectEvent— stable identity, always-fresh closure, no manual Latest Ref pattern.Cmd+Eis skipped when focus is in anINPUT|TEXTAREA|SELECT|contenteditable.Accessibility (WCAG 2.2 AA)
FieldLabel(semantic<label htmlFor>) for Crawler name and URL pattern. Schema textareas usearia-labelledbyto theirSectionTitle id.FormColumn { overflow-y: auto }; page scrolls as one unit, no nested scroll trap at 200% / 400% zoom.aria-live=\"polite\" aria-atomic=\"true\"so SR users hear count change without focus.aria-label; ModeBadge has explicit text.Robustness
inFlightRefre-entrancy guard insave()(covers rapid double-click and held Cmd+S).deriveFormState(crawler.input_schema ?? {}etc.) — even though DB isNOT NULL, the API boundary is now defensive.markSaveduses identity check to preserve user edits made between save dispatch and response.useCreateCrawler,useUpdateCrawler) verified to invalidate the crawlers list query.Review-worthy concerns / open questions
id === 'new'sentinel: chose pragmatism over a dedicated route. UUIDs can't collide with the literal string "new", and React Router's static-vs-dynamic ranking made the dual route awkward to maintain. Open to feedback.use-crawler-editor.tsreseeds form state during render when a new crawler arrives (guarded byseededForCrawlerID !== crawler.id). This is the React-documented "adjusting state when prop changes" pattern; chosen overuseEffectto avoid an extra frame of stale data. Worth a second look.Cmd+Eis suppressed in form elements. Adjust if reviewers prefer stricter behavior.Test plan
pnpm vitest run pages/CrawlerEditorPage— 16/16 passpnpm lint— clean across web workspace/crawlers/<id>and confirm view-mode read-only, Edit toggles, Save returns to view, Cancel with dirty triggers Discard dialog, test panel toggle persists across reloads/crawlers/newand confirm blank create form, Submit navigates to new detail🤖 Generated with Claude Code
Summary by CodeRabbit
릴리스 노트
새로운 기능
UI 개선
테스트