# feat: 職務経歴書フォームに未保存変更インジケーター（Dirty Dot）を追加#265
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds hierarchical dirty-state tracking and UI indicators for the career/resume form (baseline storage, diff hooks, DirtyDot component, UI wiring, tests, E2E). Implements resume PDF import pipeline: DB migration, ORM model, router, PDF extraction, LLM judge/extraction, task handler, frontend client/hook/UI/mappers, error mappings, tests, and linting/docs for message SSoT. ChangesUnsaved Changes Indicator System
Resume PDF Import (backend + frontend)
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
frontend/e2e/career-dirty-indicator.spec.ts (1)
200-201: ⚡ Quick win固定 wait は避けて条件待ちにしてください。
waitForTimeout(100)は CI 負荷で不安定化しやすいので、条件ベース（expect.pollなど）に置き換えるのが安全です。Diff suggestion
- // microtask の dispatch が消化されるまで少し待つ - await page.waitForTimeout(100); + // 固定 sleep ではなく条件待ち + await expect + .poll( + () => consoleIssues.filter((m) => m.includes("Cannot update a component")).length, + { timeout: 1500 }, + ) + .toBe(0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/e2e/career-dirty-indicator.spec.ts` around lines 200 - 201, Replace the fixed sleep (await page.waitForTimeout(100)) with a condition-based wait using Playwright's expect.poll or a page.waitForFunction-style poll: identify a stable observable that indicates microtasks have been processed (e.g., a DOM attribute/text change, a specific element state, or a small page.evaluate check that resolves only after queued microtasks run) and use expect.poll(() => page.evaluate(/* check */)).toResolve()/toEqual(...) or page.waitForFunction(...) instead of page.waitForTimeout; update the assertion call near the original wait to poll that condition until it succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/schemas/resume.py`:
- Around line 60-63: The ResumeProject model is returning non-None end_date
values (including empty strings) when is_current is True; add a Pydantic
validator (e.g., a field validator or root_validator on ResumeProject) that
checks is_current and normalizes end_date to None when is_current is True,
ensuring ResumeProject.end_date always returns None for ongoing projects and
matches Experience behavior.
In `@frontend/src/components/forms/CareerFormEditors/CareerExperienceEditor.tsx`:
- Around line 190-215: The dirty indicator can vanish when toggling
client.has_client because DirtyDot is only rendered inside the client name
block; move or duplicate the DirtyDot so it’s rendered alongside the checkbox
(outside the {client.has_client && ...} block) so clientDirty?.self remains
visible even when the name input is hidden; update the JSX around
client.has_client, the label containing clientNameLabel and the clientCheckbox
input and ensure onUpdateClientHasClient still toggles using expIndex and
clientIndex so the visible DirtyDot reflects changes to client.has_client as
well as name edits.
In `@frontend/src/hooks/career/useCareerDirty.ts`:
- Around line 99-113: buildCleanExperience currently sets every client's
projects to an empty array, which mismatches the form shape when baseline is
null and can cause nested projects[i] access errors; update buildCleanExperience
so for each client in experience.clients (in function buildCleanExperience and
the returned ExperienceDirty.clients) you preserve the per-client projects array
shape by mapping each client's actual projects length into a same-length
projects array of clean flags (e.g., map client.projects to an array of false
values) instead of always using [] so the baseline structure aligns with the
form's nested projects.
In `@frontend/src/hooks/useDocumentForm.ts`:
- Around line 146-152: The catch block currently treats any load failure as a
404 and calls commitBaseline(createInitialForm()), which can mask non-404 errors
and create a different snapshot for baseline than for the live form; change it
to inspect the caught error and only handle HTTP 404 by creating a single
initial snapshot (call createInitialForm() once into a const) and apply that
same snapshot to both the live form state (e.g., setForm or initializeForm) and
commitBaseline, while rethrowing or surfacing other errors so transient/API
failures are not treated as “no document”; update error handling inside the
catch that surrounds commitBaseline/createInitialForm accordingly.
---
Nitpick comments:
In `@frontend/e2e/career-dirty-indicator.spec.ts`:
- Around line 200-201: Replace the fixed sleep (await page.waitForTimeout(100))
with a condition-based wait using Playwright's expect.poll or a
page.waitForFunction-style poll: identify a stable observable that indicates
microtasks have been processed (e.g., a DOM attribute/text change, a specific
element state, or a small page.evaluate check that resolves only after queued
microtasks run) and use expect.poll(() => page.evaluate(/* check
*/)).toResolve()/toEqual(...) or page.waitForFunction(...) instead of
page.waitForTimeout; update the assertion call near the original wait to poll
that condition until it succeeds.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 150cbe1d-9467-495b-a033-e21c8e388bc8
📒 Files selected for processing (23)
.claude/CLAUDE.md.claude/rules/backend/auth-security.md.claude/rules/backend/database.mdbackend/app/schemas/resume.pybackend/tests/test_schemas.pyfrontend/e2e/career-dirty-indicator.spec.tsfrontend/src/components/forms/CareerFormEditors/CareerExperienceEditor.tsxfrontend/src/components/forms/CareerResumeForm.tsxfrontend/src/components/forms/MarkdownTextarea.tsxfrontend/src/components/forms/ProjectModal.tsxfrontend/src/components/forms/sections/CareerBasicInfoSection.tsxfrontend/src/components/forms/sections/CareerExperienceSection.tsxfrontend/src/components/forms/sections/CareerQualificationsSection.tsxfrontend/src/components/forms/sections/CareerSelfPrSection.tsxfrontend/src/components/ui/DirtyDot.module.cssfrontend/src/components/ui/DirtyDot.tsxfrontend/src/hooks/career/useCareerDirty.test.tsfrontend/src/hooks/career/useCareerDirty.tsfrontend/src/hooks/career/useProjectFormDirty.test.tsfrontend/src/hooks/career/useProjectFormDirty.tsfrontend/src/hooks/useDocumentForm.tsfrontend/src/store/formCacheSlice.tsinfra/modules/cloud_run/main.tf
💤 Files with no reviewable changes (3)
- infra/modules/cloud_run/main.tf
- .claude/CLAUDE.md
- .claude/rules/backend/auth-security.md
Comment on lines
146
to
152
| } catch { | ||
| if (!active) return; | ||
| // DB に未登録（404）のユーザー向け: 初期空フォームを baseline として確定する。 | ||
| // これにより以後のユーザー編集はすべて baseline との差分として検出され、 | ||
| // 各フィールド・配下要素（プロジェクト等）の未保存マークが正しく表示される。 | ||
| commitBaseline(createInitialForm()); | ||
| } finally { |
There was a problem hiding this comment.
Handle non-404 load failures separately, and commit the exact same initial snapshot to both form and baseline.
Line 146 currently treats every load failure as “no existing document”. That can mask transient/API failures and later overwrite existing data. Also, Line 151 creates a fresh initial snapshot only for baseline, which can desync from current form if createInitialForm() is non-deterministic.
💡 Suggested fix
- } catch {
+ } catch (e) {
if (!active) return;
- // DB に未登録（404）のユーザー向け: 初期空フォームを baseline として確定する。
- // これにより以後のユーザー編集はすべて baseline との差分として検出され、
- // 各フィールド・配下要素（プロジェクト等）の未保存マークが正しく表示される。
- commitBaseline(createInitialForm());
+ // 404 のみ「未登録」とみなして初期状態を確定する
+ // （判定は既存 API エラーモデルに合わせて実装）
+ if (isNotFoundError(e)) {
+ const initial = createInitialForm();
+ setFormRaw(initial);
+ commitBaseline(initial);
+ } else {
+ const message =
+ e instanceof Error ? e.message : "読み込み中に不明なエラーが発生しました。";
+ setError(message);
+ }
} finally {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/hooks/useDocumentForm.ts` around lines 146 - 152, The catch
block currently treats any load failure as a 404 and calls
commitBaseline(createInitialForm()), which can mask non-404 errors and create a
different snapshot for baseline than for the live form; change it to inspect the
caught error and only handle HTTP 404 by creating a single initial snapshot
(call createInitialForm() once into a const) and apply that same snapshot to
both the live form state (e.g., setForm or initializeForm) and commitBaseline,
while rethrowing or surfacing other errors so transient/API failures are not
treated as “no document”; update error handling inside the catch that surrounds
commitBaseline/createInitialForm accordingly.
Owner
Author
職務経歴書 PDF インポート機能の追加Summary
変更点Backend
Frontend
Tests
Test plan
関連 ADR / 設計判断
|
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (5)
frontend/src/api/paths.ts (1)
74-78: 💤 Low valueConsider nesting under
PATHS.resumesfor consistency.The new
resumeImportsendpoints start with/api/resumes/import, which suggests they logically belong to the resumes resource. Organizing them asPATHS.resumes.importrather than a top-levelPATHS.resumeImportswould make the hierarchy more intuitive and consistent with the URL structure.If the separation is intentional to mirror the backend router organization, consider adding a comment explaining the reasoning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/api/paths.ts` around lines 74 - 78, The resumeImports endpoints are defined at top-level as PATHS.resumeImports but their URLs start with /api/resumes/import and should be nested under the existing PATHS.resumes for consistency; move the resumeImports object into PATHS.resumes as an import property (e.g., PATHS.resumes.import.start, .status(id), .result(id)) or, if intentional, add a clarifying comment near the PATHS.resumeImports definition explaining why it is kept top-level to mirror backend routing.frontend/src/api/resumeImports.ts (2)
58-58: ⚡ Quick winResponse type casting lacks runtime validation.
The cast to
Promise<ResumeImportStartResponse>provides compile-time safety but no runtime validation. If the API returns an unexpected structure (e.g., due to a backend bug or API version mismatch), the error will surface later when accessingimport_id, making debugging harder.Consider adding runtime validation using a schema validator (e.g., Zod) or at minimum an assertion that checks for required fields.
♻️ Example with basic validation
- return response.json() as Promise<ResumeImportStartResponse>; + const data = await response.json(); + if (!data.import_id || typeof data.import_id !== 'string') { + throw new ApiError({ + code: 'INTERNAL_ERROR', + message: 'Invalid response from server', + action: null, + }); + } + return data as ResumeImportStartResponse;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/api/resumeImports.ts` at line 58, The current return casts response.json() to Promise<ResumeImportStartResponse> without runtime checks; update the code that returns response.json() to validate the parsed payload before returning (e.g., use a Zod schema or a small assertion) to ensure required fields like import_id exist and have the expected types, and throw or return a typed error if validation fails; reference the ResumeImportStartResponse shape and the response.json() call so you validate the parsed object and only then return it as a ResumeImportStartResponse.
33-33: 💤 Low valueConsider using a dedicated cookie parsing utility.
The regex pattern works but is simplistic. If cookies have URL-encoded values or complex formats, it may not extract the token correctly. Consider using a dedicated cookie parsing function for more robust handling.
♻️ Example using a helper function
function getCookie(name: string): string { const value = `; ${document.cookie}`; const parts = value.split(`; ${name}=`); if (parts.length === 2) return parts.pop()?.split(';').shift() ?? ''; return ''; } const csrfToken = getCookie('csrf_token');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/api/resumeImports.ts` at line 33, Replace the simplistic regex cookie extraction for csrfToken with a robust cookie parsing helper: add a getCookie(name: string) function and use const csrfToken = getCookie('csrf_token'); update any direct uses of document.cookie in resumeImports.ts to call getCookie('csrf_token') so URL-encoded or complex cookie values are handled correctly (refer to the csrfToken constant and implement getCookie in the same module or a shared util).frontend/src/components/forms/ResumeImportConfirmModal.tsx (1)
31-33: ⚡ Quick winAdd dialog semantics to the modal container.
Line 32 renders a modal visually, but it lacks
role="dialog",aria-modal="true", and an accessible label link (e.g.,aria-labelledbyto the title at Line 33). This improves screen reader navigation without changing behavior.Proposed patch
- <div className={styles.overlay} onClick={onCancel}> - <div className={styles.modal} onClick={(e) => e.stopPropagation()}> - <h2 className={styles.title}>PDF から読み取った内容</h2> + <div className={styles.overlay} onClick={onCancel}> + <div + className={styles.modal} + role="dialog" + aria-modal="true" + aria-labelledby="resume-import-confirm-title" + onClick={(e) => e.stopPropagation()} + > + <h2 id="resume-import-confirm-title" className={styles.title}>PDF から読み取った内容</h2>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/forms/ResumeImportConfirmModal.tsx` around lines 31 - 33, The modal container rendered in ResumeImportConfirmModal is missing dialog semantics; update the div with className={styles.modal} to include role="dialog", aria-modal="true", and aria-labelledby that points to the modal title, and add a stable id to the title element (the h2 with className={styles.title}) so screen readers can reference it; keep the existing onClick={(e) => e.stopPropagation()} and onCancel handler unchanged.backend/tests/test_resume_import_handler.py (1)
124-128: ⚡ Quick winAssert terminal status in failure-path tests as well.
These tests already check error details and blob cleanup; adding
status == "dead_letter"assertions will lock in expected terminal-state behavior.Also applies to: 151-154
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_resume_import_handler.py` around lines 124 - 128, Add an assertion that the ResumeImport record reaches the terminal failure status by asserting record.status == "dead_letter" after refreshing the record in the failure-path tests (the block using db_session.refresh(record) and asserting record.error_message and record.pdf_blob). Apply the same addition for the other failure test mentioned (the similar block around lines 151-154) so both tests explicitly check the terminal status on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/alembic_migrations/versions/0032_add_resume_imports.py`:
- Around line 46-51: The migration sets updated_at with only
server_default=sa.func.now() while the model (resume_import.py) relies on
SQLAlchemy's onupdate=func.now(); to cover non-ORM updates add a DB-side UPDATE
mechanism: modify the migration (0032_add_resume_imports.py) to create a trigger
function that sets NEW.updated_at = now() on row UPDATE and attach a trigger to
the resume_imports table in the upgrade() and drop it in downgrade(); keep the
existing Column(...) definition but add the trigger creation SQL (and trigger
drop) so updated_at is updated for raw SQL/other clients as well.
In `@backend/app/routers/resume_imports.py`:
- Around line 56-64: The current code awaits file.read() and loads the entire
upload into pdf_bytes before checking _MAX_FILE_SIZE; change to stream the
upload in chunks (e.g., repeatedly call await file.read(CHUNK_SIZE) and append
into a bytearray buffer) while keeping a running total and immediately call
raise_app_error (same status_code, code, message, action) and stop reading/close
the file if the total exceeds _MAX_FILE_SIZE; use the existing names (file,
pdf_bytes/buffer, _MAX_FILE_SIZE, raise_app_error) so the size check happens
incrementally and large uploads never get fully loaded into memory.
- Around line 165-168: Guard the JSON parsing of record.result_json to avoid
uncaught exceptions: instead of directly calling parsed =
json.loads(record.result_json), first check that record.result_json is truthy
and wrap json.loads in a try/except catching json.JSONDecodeError (and Exception
as fallback); on failure return a safe domain response (e.g.,
ResumeImportResultResponse with result set to None or a default ResumeBase
placeholder) and preserve record.is_resume_flag, and optionally log the error;
update the code around ResumeImportResultResponse / ResumeBase to accept the
safe fallback.
- Around line 97-112: When dispatching the resume import task via
AsyncTaskCacheService.dispatch (TaskType.RESUME_IMPORT) fails, update the
persisted import record to mark it as a terminal/failed state and clear its
pdf_blob to avoid leaving sensitive data and stuck rows; catch the Exception
around service.dispatch, set record.status (or the model's terminal/status
field) to the terminal error value and set record.pdf_blob = None (or empty),
persist the change via the same db/session used by AsyncTaskCacheService (e.g.,
db.add/db.commit or record.save), then log the change and re-raise the HTTP 500
via raise_app_error as before. Ensure you reference AsyncTaskCacheService,
dispatch, TaskType.RESUME_IMPORT, record, and pdf_blob when locating the code to
modify.
In `@backend/app/services/resume_import/llm_extractor.py`:
- Line 53: The LLM calls to llm_client.generate (invoked with system_prompt and
user_prompt) need explicit timeouts to avoid hanging; update both call sites
(the generate at raw = await llm_client.generate(system_prompt, user_prompt) and
the similar call around line 84) to either pass a supported timeout argument to
LLMClient.generate or wrap the await in asyncio.wait_for with a sensible timeout
(e.g., 30s), catch asyncio.TimeoutError and re-raise a RetryableError (or the
module's retryable exception) with a clear message so the worker can recover
instead of blocking indefinitely.
- Line 63: The current logger.warning call exposes PII by printing raw[:200];
update the logging in llm_extractor.py to avoid emitting resume content by
replacing the raw snippet with non-PII metadata (e.g., log the response length
using len(raw), a status code or parsing outcome, or a fixed message). Locate
the logger.warning usage(s) (the call referencing raw and the logger in this
module, also the similar occurrence around lines 91) and change the message to
something like "LLM 判定レスポンスのパースに失敗しました: response length=%d" or a fixed warning
string so no personal data from raw is written to logs.
In `@backend/app/services/resume_import/pdf_extractor.py`:
- Around line 30-43: The code currently treats an empty pages list as having a
text layer; update the logic in pdf_extractor.py to explicitly handle an empty
extracted page set by setting has_text_layer = False (and full_text = "" if
needed) when pages is empty; locate the block using pdf.pages, pages,
sparse_pages, _MIN_CHARS_PER_PAGE, full_text and has_text_layer and add an early
check like "if not pages: full_text = ''; has_text_layer = False" before
computing sparse_pages and the existing half-pages condition.
In `@backend/app/services/resume_import/prompts/extract_resume.md`:
- Line 47: The phases values are inconsistent between the JSON schema's "phases"
array and the extraction rules that list separate design phases; update them to
match. Pick one approach (recommended: expand the schema) and edit the "phases"
enum/array to include "要件定義", "基本設計", "詳細設計", "開発", "テスト", "リリース", "保守運用", and
ensure the extraction instruction text that currently lists
「要件定義」「基本設計」「詳細設計」「開発」「テスト」「リリース」「保守運用」 exactly matches those strings; also
update any validation logic or enums that reference "phases" (e.g.,
extraction/validation routines) to use the same set.
In `@backend/app/services/tasks/handlers/resume_import.py`:
- Around line 62-83: The scan-PDF and not-a-resume branches update record fields
then commit and raise NonRetryableError but do not set the terminal status
field, leaving status at "processing"; update both branches (the
extracted.has_text_layer false branch and the if not judge_result.is_resume
branch around llm_extractor.judge_is_resume) to assign the same terminal status
value used by the missing-PDF path (e.g., record.status = "failed" or the
project's defined terminal status), then set record.is_resume_flag,
record.judge_reason, record.error_message, record.pdf_blob, record.completed_at,
call db.commit(), and raise NonRetryableError as before so the terminal status
is persisted.
In `@backend/requirements.txt`:
- Around line 28-29: Tighten the python-multipart requirement in
backend/requirements.txt to avoid known vulnerabilities by changing the lower
bound from python-multipart>=0.0.9 to at least python-multipart>=0.0.27 (or pin
to a known safe version such as 0.0.29); edit the requirements entry for
python-multipart so dependency resolution will not install vulnerable versions
while leaving pdfplumber>=0.11.0 unchanged.
In `@frontend/src/components/forms/CareerResumeForm.tsx`:
- Line 118: The isDirty prop is computed from
Object.values(dirty).some(Boolean), which can return true for non-boolean truthy
entries and thus show dirty even when nothing changed; update the aggregation to
only consider explicit boolean flags (e.g. replace
Object.values(dirty).some(Boolean) with Object.values(dirty).some(v => v ===
true)) or introduce/maintain a single boolean like hasUnsavedChanges and pass
that to isDirty; look for the dirty variable and the isDirty prop usage in
CareerResumeForm (the component) and change the expression to only consider true
booleans or an aggregated boolean flag.
In `@frontend/src/formMappers.ts`:
- Around line 22-29: The blank-detection in _isBlankExperiences currently only
checks company, business_description and start_date, which can mark
partially-filled entries as blank and overwrite user data; update
_isBlankExperiences to consider all form fields for the single experience (e.g.,
use a check that iterates over the experience object's string properties and
returns true only if every property .trim() is empty) and apply the same fix to
the other blank-detection logic referenced later in the file so that only truly
empty experience objects are treated as blank.
- Around line 59-60: The current check treats importedForm.experiences as empty
if every experience has an empty company, which can discard useful data in other
fields; change the predicate used in the if so it only treats an experience as
empty when all relevant fields are blank (e.g., company, title, period,
description) — either inline the check or add a helper like
isExperienceEmpty(ex) that trims and tests all fields, and use
importedForm.experiences.every(isExperienceEmpty) before substituting
existing.experiences.
In `@frontend/src/hooks/career/useResumeImport.ts`:
- Around line 94-97: The polling race is caused by setImportId being async so
checkStatus (used by startPolling) reads a stale importId; replace the importId
state with a ref (e.g., importIdRef) and update importIdRef.current = import_id
inside the start/resume logic (the code around startResumeImport and start
functions), then have checkStatus/readers reference importIdRef.current instead
of the importId state; also update reset to clear importIdRef.current and remove
or keep a minimal state-only representation for UI if needed so startPolling
always sees the current import id.
---
Nitpick comments:
In `@backend/tests/test_resume_import_handler.py`:
- Around line 124-128: Add an assertion that the ResumeImport record reaches the
terminal failure status by asserting record.status == "dead_letter" after
refreshing the record in the failure-path tests (the block using
db_session.refresh(record) and asserting record.error_message and
record.pdf_blob). Apply the same addition for the other failure test mentioned
(the similar block around lines 151-154) so both tests explicitly check the
terminal status on failure.
In `@frontend/src/api/paths.ts`:
- Around line 74-78: The resumeImports endpoints are defined at top-level as
PATHS.resumeImports but their URLs start with /api/resumes/import and should be
nested under the existing PATHS.resumes for consistency; move the resumeImports
object into PATHS.resumes as an import property (e.g.,
PATHS.resumes.import.start, .status(id), .result(id)) or, if intentional, add a
clarifying comment near the PATHS.resumeImports definition explaining why it is
kept top-level to mirror backend routing.
In `@frontend/src/api/resumeImports.ts`:
- Line 58: The current return casts response.json() to
Promise<ResumeImportStartResponse> without runtime checks; update the code that
returns response.json() to validate the parsed payload before returning (e.g.,
use a Zod schema or a small assertion) to ensure required fields like import_id
exist and have the expected types, and throw or return a typed error if
validation fails; reference the ResumeImportStartResponse shape and the
response.json() call so you validate the parsed object and only then return it
as a ResumeImportStartResponse.
- Line 33: Replace the simplistic regex cookie extraction for csrfToken with a
robust cookie parsing helper: add a getCookie(name: string) function and use
const csrfToken = getCookie('csrf_token'); update any direct uses of
document.cookie in resumeImports.ts to call getCookie('csrf_token') so
URL-encoded or complex cookie values are handled correctly (refer to the
csrfToken constant and implement getCookie in the same module or a shared util).
In `@frontend/src/components/forms/ResumeImportConfirmModal.tsx`:
- Around line 31-33: The modal container rendered in ResumeImportConfirmModal is
missing dialog semantics; update the div with className={styles.modal} to
include role="dialog", aria-modal="true", and aria-labelledby that points to the
modal title, and add a stable id to the title element (the h2 with
className={styles.title}) so screen readers can reference it; keep the existing
onClick={(e) => e.stopPropagation()} and onCancel handler unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ec81d4a-78d4-4a3a-aae5-2e6f15301ade
📒 Files selected for processing (39)
.claude/rules/security.md.vscode/settings.jsonbackend/alembic_migrations/versions/0032_add_resume_imports.pybackend/app/core/errors.pybackend/app/main.pybackend/app/models/__init__.pybackend/app/models/resume_import.pybackend/app/routers/__init__.pybackend/app/routers/resume_imports.pybackend/app/schemas/resume.pybackend/app/schemas/resume_import.pybackend/app/services/resume_import/__init__.pybackend/app/services/resume_import/llm_extractor.pybackend/app/services/resume_import/pdf_extractor.pybackend/app/services/resume_import/prompts/extract_resume.mdbackend/app/services/resume_import/prompts/judge_resume.mdbackend/app/services/tasks/base.pybackend/app/services/tasks/handlers/__init__.pybackend/app/services/tasks/handlers/resume_import.pybackend/requirements.txtbackend/tests/conftest.pybackend/tests/test_resume_import_handler.pybackend/tests/test_resume_imports_router.pybackend/tests/test_schemas.pyfrontend/src/api/paths.tsfrontend/src/api/resumeImports.tsfrontend/src/components/forms/CareerFormEditors/CareerExperienceEditor.tsxfrontend/src/components/forms/CareerResumeForm.tsxfrontend/src/components/forms/ImportResumeButton.tsxfrontend/src/components/forms/ResumeImportConfirmModal.module.cssfrontend/src/components/forms/ResumeImportConfirmModal.tsxfrontend/src/constants/errorCodes.tsfrontend/src/constants/errorMessages.tsfrontend/src/formMappers.mergeImportedResume.test.tsfrontend/src/formMappers.tsfrontend/src/hooks/career/__tests__/useResumeImport.test.tsfrontend/src/hooks/career/useCareerDirty.test.tsfrontend/src/hooks/career/useCareerDirty.tsfrontend/src/hooks/career/useResumeImport.ts
✅ Files skipped from review due to trivial changes (4)
- backend/app/services/resume_import/prompts/judge_resume.md
- backend/app/models/init.py
- backend/app/services/resume_import/init.py
- backend/app/services/tasks/base.py
Comment on lines
+46
to
+51
| sa.Column( | ||
| "updated_at", | ||
| sa.DateTime(timezone=True), | ||
| server_default=sa.func.now(), | ||
| nullable=False, | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the ORM model defines onupdate for updated_at
rg -A 5 "class ResumeImport" --type py --glob '**/models/resume_import.py'
# Look for updated_at column definition in the model
rg "updated_at.*Column" --type py --glob '**/models/resume_import.py' -A 2Repository: yusuke0610/devforge
Length of output: 440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the migration's updated_at column definition
echo "=== Migration: updated_at column ==="
sed -n '1,120p' backend/alembic_migrations/versions/0032_add_resume_imports.py | nl -ba | sed -n '35,80p' || true
# Inspect the model for ResumeImport.updated_at and any onupdate/onupdate events
echo "=== Model: backend/app/models/resume_import.py (updated_at occurrences) ==="
rg -n "updated_at" backend/app/models/resume_import.py || true
echo "=== Model: lines around updated_at definition ==="
rg -n "updated_at" -n backend/app/models/resume_import.py -n --context 10 || true
# Also scan for mapper events / SQLAlchemy event listeners that might update updated_at
echo "=== Model: SQLAlchemy event hooks near ResumeImport ==="
rg -n "event\.listen|mapper|before_update|onupdate" backend/app/models/resume_import.py || trueRepository: yusuke0610/devforge
Length of output: 1135
Fix the updated_at auto-update concern—ORM already updates it
backend/app/alembic_migrations/versions/0032_add_resume_imports.py defines updated_at with only server_default=sa.func.now() (no DB-side update trigger), but backend/app/models/resume_import.py sets updated_at with onupdate=func.now(), so SQLAlchemy ORM-generated UPDATEs will refresh updated_at.
If resume_imports rows can be modified outside the ORM (raw SQL/other jobs), updated_at won’t auto-update in that path—add a DB trigger if that’s a requirement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/alembic_migrations/versions/0032_add_resume_imports.py` around lines
46 - 51, The migration sets updated_at with only server_default=sa.func.now()
while the model (resume_import.py) relies on SQLAlchemy's onupdate=func.now();
to cover non-ORM updates add a DB-side UPDATE mechanism: modify the migration
(0032_add_resume_imports.py) to create a trigger function that sets
NEW.updated_at = now() on row UPDATE and attach a trigger to the resume_imports
table in the upgrade() and drop it in downgrade(); keep the existing Column(...)
definition but add the trigger creation SQL (and trigger drop) so updated_at is
updated for raw SQL/other clients as well.
Owner
Author
There was a problem hiding this comment.
スキップしました — このコードベースでは検出結果が無効です。
理由：
-
データベースエンジンの不一致。データベースはTurso/libSQL（SQLite互換）であり、PostgreSQLではありません。提案された修正では、PL/pgSQLのトリガー関数構文（CREATE FUNCTION内のNEW.updated_at = now()）が使用されていますが、libSQLはこの構文をサポートしていません。SQLiteのトリガーは異なる形式を使用しており、backend/alembic_migrations/versions/ のマイグレーションではトリガーは一切使用されていません。これは確立されたパターンからの例外的な変更となります。
-
ORM以外のライターが存在しません。resume_imports / ResumeImportをgrepした結果、すべての書き込みパスがSQLAlchemy ORM（routers/resume_imports.py、
services/tasks/handlers/resume_import.py、およびdb_session経由のテスト）を経由していることが分かりました。 ORMのみを使用するライターの場合、モデルのonupdate=func.now() (resume_import.py:57) は更新のたびに実行されます。
この発見で想定されている「生のSQL/その他のクライアント」は存在しません。
- 兄弟テーブルとの一貫性。blog.py、cache.py、resume.pyはすべて、データベース側のトリガーなしで、onupdate=func.now()のみを使用する同じパターンを使用しています。resume_importsにのみトリガーを追加すると、
実際の問題を解決することなく、一貫性のない規約を作成することになります。
コードの変更はありません。検証対象はありません。
There was a problem hiding this comment.
@yusuke0610 ご説明ありがとうございます。3点の理由はいずれも正確で、私の指摘は誤りでした。Turso/libSQL（SQLite 互換）環境では PL/pgSQL のトリガー構文は使用できず、書き込みパスもすべて SQLAlchemy ORM 経由であるため onupdate=func.now() で十分に機能します。また、既存テーブルとの一貫性を保つ判断も理にかなっています。失礼しました。
(´・ω・`)
✏️ Learnings added
Learnt from: yusuke0610
Repo: yusuke0610/devforge PR: 265
File: backend/alembic_migrations/versions/0032_add_resume_imports.py:46-51
Timestamp: 2026-05-22T06:21:50.368Z
Learning: The database engine for this project (yusuke0610/devforge) is Turso/libSQL (SQLite-compatible), NOT PostgreSQL. PostgreSQL-specific features such as PL/pgSQL trigger functions (CREATE FUNCTION ... RETURNS TRIGGER) are not supported. All migration files under backend/alembic_migrations/versions/ use SQLite-compatible SQL only, and no DB-side triggers are used anywhere in the codebase. All write paths go through the SQLAlchemy ORM, so onupdate=func.now() in model definitions (e.g., backend/app/models/) is sufficient for auto-updating timestamp columns like updated_at. Sibling models (blog.py, cache.py, resume.py) all follow this same pattern without DB-side triggers.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+165
to
+168
| parsed = json.loads(record.result_json) | ||
| return ResumeImportResultResponse( | ||
| result=ResumeBase(**parsed), | ||
| is_resume=bool(record.is_resume_flag), |
There was a problem hiding this comment.
Guard result_json parsing to avoid uncontrolled 500s.
json.loads(record.result_json) assumes a valid non-null payload. If DB state is inconsistent, this throws and returns an internal error without domain-specific handling.
Proposed fix
- parsed = json.loads(record.result_json)
+ try:
+ if not record.result_json:
+ raise ValueError("empty result_json")
+ parsed = json.loads(record.result_json)
+ except Exception:
+ raise_app_error(
+ status_code=409,
+ code=ErrorCode.VALIDATION_ERROR,
+ message="抽出結果がまだ利用可能ではありません。",
+ action="しばらく待ってから再試行してください",
+ )📝 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.
Suggested change
| parsed = json.loads(record.result_json) | |
| return ResumeImportResultResponse( | |
| result=ResumeBase(**parsed), | |
| is_resume=bool(record.is_resume_flag), | |
| try: | |
| if not record.result_json: | |
| raise ValueError("empty result_json") | |
| parsed = json.loads(record.result_json) | |
| except Exception: | |
| raise_app_error( | |
| status_code=409, | |
| code=ErrorCode.VALIDATION_ERROR, | |
| message="抽出結果がまだ利用可能ではありません。", | |
| action="しばらく待ってから再試行してください", | |
| ) | |
| return ResumeImportResultResponse( | |
| result=ResumeBase(**parsed), | |
| is_resume=bool(record.is_resume_flag), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/routers/resume_imports.py` around lines 165 - 168, Guard the JSON
parsing of record.result_json to avoid uncaught exceptions: instead of directly
calling parsed = json.loads(record.result_json), first check that
record.result_json is truthy and wrap json.loads in a try/except catching
json.JSONDecodeError (and Exception as fallback); on failure return a safe
domain response (e.g., ResumeImportResultResponse with result set to None or a
default ResumeBase placeholder) and preserve record.is_resume_flag, and
optionally log the error; update the code around ResumeImportResultResponse /
ResumeBase to accept the safe fallback.
Owner
Author
There was a problem hiding this comment.
スキップ — 検出結果が明示的なプロジェクトルールと強力なドメイン不変条件に矛盾するため。
理由：
- 不変条件：status=="completed" ⇒ 有効な result_json。status = "completed" を設定する唯一のライターは services/tasks/handlers/resume_import.py:87-94 であり、record.result_json =
json.dumps(structured, ensure_ascii=False) は record.status = "completed" と同じコミットで設定されています。有効な json.dumps 出力を書き込まずにレコードを completed に昇格させるパスは存在しません。
resume_imports.py:179 の if record.status != "completed" ガードは、他のすべての状態を 409 エラーで拒否しています。したがって、json.loads(record.result_json) は
ここで失敗するはずがありません。
- この防御策に対する明示的なプロジェクトルール。 .claude/CLAUDE.md（タスクの実行セクション）：「発生し得ないシナリオに対して、エラー処理、フォールバック、または検証を追加しないでください。
内部コードとフレームワークの保証を信頼してください。検証はシステム境界（ユーザー入力、外部API）でのみ行ってください。」 record.result_json は、ステータスの反転と同じトランザクションで、独自のハンドラによって書き込まれる内部データであり、境界ではありません。
- 提案されている「安全なフォールバック」は事態を悪化させます。ResumeImportResultResponse.result は ResumeBase 型です（必須、schemas/resume_import.py:28）。result=None を返すと、
スキーマを ResumeBase | None に拡張する必要があり、すべてのフロントエンド呼び出し元が、契約上存在し得ない「完了したが結果がない」状態を処理することを強制されます。さらに悪いことに、
実際のデータ破損を、アラートをトリガーする 500 として顕在化させる代わりに、サイレント 200 で隠蔽することになります。
- 現在の動作は、失敗時に既に正しく動作しています。 result_jsonが何らかの理由で無効になった場合（データベースの破損、手動による介入など）、FastAPIは捕捉されなかったJSONDecodeErrorを500エラーに変換します。
これは望ましい動作です。不変条件違反を隠蔽するのではなく、表面化させるべきです。
There was a problem hiding this comment.
@yusuke0610 ご説明ありがとうございます。すべての点に同意します。
- 不変条件の保証：
result_jsonとstatus="completed"が同一トランザクション内で書き込まれることは、コードから明確に確認できます。 - プロジェクトルールの優先：
.claude/CLAUDE.mdの「発生し得ないシナリオに対してエラー処理を追加しない」という方針は合理的であり、今回のケースに正確に該当します。 - フォールバックの弊害：
result=Noneを許容するとスキーマ・フロントエンド双方に不要な複雑性が生じ、実際の不整合を隠蔽するリスクもあります。
この提案は取り下げます。
🐇
✏️ Learnings added
Learnt from: yusuke0610
Repo: yusuke0610/devforge PR: 265
File: backend/app/routers/resume_imports.py:165-168
Timestamp: 2026-05-22T11:50:08.476Z
Learning: In the `devforge` repository, the project rule (defined in `.claude/CLAUDE.md`) explicitly prohibits adding error handling, fallbacks, or validation for scenarios that cannot occur. Internal data written by the system's own handlers in the same DB transaction is trusted without defensive guards. Validation is only applied at system boundaries (user input, external APIs). Example: `json.loads(record.result_json)` in `backend/app/routers/resume_imports.py` is intentionally unguarded because `result_json` is always set valid in the same commit that sets `status="completed"`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| system_prompt = _load_prompt("judge_resume.md") | ||
| user_prompt = text[:_JUDGE_TEXT_MAX_CHARS] | ||
|
|
||
| raw = await llm_client.generate(system_prompt, user_prompt) |
There was a problem hiding this comment.
Consider adding timeout to LLM calls.
The LLM generate() calls lack explicit timeout parameters. If the LLM service becomes unresponsive, these calls could block indefinitely, tying up worker resources.
While llm_client might have internal timeouts, making them explicit here would improve reliability and make the behavior more predictable.
⏱️ Example: Add timeout parameter
-raw = await llm_client.generate(system_prompt, user_prompt)
+raw = await llm_client.generate(system_prompt, user_prompt, timeout=30)Note: Verify that LLMClient.generate() supports a timeout parameter. If not, consider wrapping the call with asyncio.wait_for():
import asyncio
try:
raw = await asyncio.wait_for(
llm_client.generate(system_prompt, user_prompt),
timeout=30.0
)
except asyncio.TimeoutError:
raise RetryableError("LLM 呼び出しがタイムアウトしました")Also applies to: 84-84
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/services/resume_import/llm_extractor.py` at line 53, The LLM
calls to llm_client.generate (invoked with system_prompt and user_prompt) need
explicit timeouts to avoid hanging; update both call sites (the generate at raw
= await llm_client.generate(system_prompt, user_prompt) and the similar call
around line 84) to either pass a supported timeout argument to
LLMClient.generate or wrap the await in asyncio.wait_for with a sensible timeout
(e.g., 30s), catch asyncio.TimeoutError and re-raise a RetryableError (or the
module's retryable exception) with a clear message so the worker can recover
instead of blocking indefinitely.
Owner
Author
There was a problem hiding this comment.
スキップ — タイムアウトは既に適切なレイヤーで再試行可能なセマンティクスで実装されており、提案された30秒のラッパーは設定値と競合します。
理由：
- OllamaClientは既にタイムアウトを強制しています。ollama_client.py:34では、HTTP呼び出しをhttpx.AsyncClient(timeout=self.timeout)でラップしています。ここでself.timeoutはOLLAMA_TIMEOUT（デフォルト値は1200秒、
ollama_client.py:24で低速なローカルLLM向けに意図的に設定）です。httpx.TimeoutExceptionはollama_client.py:55-66で捕捉され、RetryableError("Ollama タイムアウト: ...")として再スローされます。
これはまさに、今回の検出で求められているリカバリパスです。
- VertexClientは既にタイムアウトを強制しています。 vertex_client.py:107-113 は、google-genai SDK から asyncio.TimeoutError / TimeoutError をキャッチし、RetryableError("Vertex AI
遅延: ...") として再発生させます。SDK はさらに gax.DeadlineExceeded を発生させますが、_classify_vertex_exception (vertex_client.py:164-177) はこれを RetryableError にマッピングします。
- 提案されている 30 秒のラッパーはこのドメインには不適切です。Ollama は gemma3:4b をローカルで実行していますが、コールドスタートと最初のトークン取得の遅延が 30 秒を超えることが頻繁に発生するため、環境のデフォルト値は 1200 秒になっています。
呼び出し箇所を asyncio.wait_for(..., timeout=30) でラップすると、正当な呼び出しが時期尚早に中断され、誤った RetryableError が発生し、リトライ予算が無駄に消費されます。 LLMClient.generate コントラクトは、呼び出しごとのタイムアウト パラメータを意図的に公開していません。タイムアウト ポリシーは、環境変数 (OLLAMA_TIMEOUT、Vertex SDK の deadline) を介してプロバイダごとに設定されます。
- 無限にハングアップするパスは存在しません。両方のプロバイダが接続/読み取り/プールのタイムアウトを処理します。ワーカー レイヤー (services/tasks/worker.py) も独自のタスク レベルのタイムアウト処理を備えています (.claude/rules/backend/test.md で参照されている tests/test_worker_timeout.py を参照)。llm_extractor 内に 3 番目のタイムアウト レイヤーを追加すると冗長になり、どのレイヤーがタイムアウトを発生させたのかが不明瞭になります。
コードの変更はありません。検証対象はありません。
There was a problem hiding this comment.
@yusuke0610 理解しました。ご説明ありがとうございます。
確認させてください：
- OllamaClient は
httpx.AsyncClient(timeout=self.timeout)でOLLAMA_TIMEOUT（デフォルト 1200 秒）を適用し、httpx.TimeoutExceptionをRetryableErrorに変換済み。 - VertexClient は
asyncio.TimeoutError/gax.DeadlineExceededをRetryableErrorにマッピング済み。 - ワーカー層にも独自のタスクレベルタイムアウトが存在する。
これらを踏まえると、llm_extractor.py 内に 30 秒ラッパーを追加することは冗長かつ有害（ローカル LLM のコールドスタートを誤って中断する）であることは明らかです。コメントは取り下げます。
✏️ Learnings added
Learnt from: yusuke0610
Repo: yusuke0610/devforge PR: 265
File: backend/app/services/resume_import/llm_extractor.py:53-53
Timestamp: 2026-05-22T11:51:44.629Z
Learning: In yusuke0610/devforge, LLM timeout handling is implemented at the provider layer, not at the call site in llm_extractor.py. OllamaClient (backend/app/services/intelligence/llm/ollama_client.py) wraps HTTP calls with httpx.AsyncClient(timeout=self.timeout) where self.timeout=OLLAMA_TIMEOUT (default 1200s for slow local LLMs), and converts httpx.TimeoutException to RetryableError. VertexClient (backend/app/services/intelligence/llm/vertex_client.py) catches asyncio.TimeoutError and gax.DeadlineExceeded and maps them to RetryableError. The worker layer (services/tasks/worker.py) also has task-level timeout handling. Do NOT suggest adding asyncio.wait_for or per-call timeout wrappers in llm_extractor.py or any LLMClient call site — this would conflict with intentional provider-level timeout configuration and cause premature cancellation of legitimate slow LLM calls.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: yusuke0610
Repo: yusuke0610/devforge PR: 265
File: backend/app/routers/resume_imports.py:165-168
Timestamp: 2026-05-22T11:50:08.476Z
Learning: Follow the devforge project rule in `.claude/CLAUDE.md`: during code review, do NOT ask for extra error handling/fallbacks/defensive validation for states that are guaranteed not to occur by system invariants. Treat data written by the system’s own handlers within the same DB transaction (and tied to the same commit/invariant—e.g., fields associated with `status="completed"`) as trusted. Apply validation only at system boundaries (user input and external APIs) or when data can change across transaction boundaries / originate outside the trusted handler path. Example: keep `json.loads(record.result_json)` unguarded in `backend/app/routers/resume_imports.py` when `result_json` is guaranteed valid by the same handler that sets the corresponding completion status.
Comment on lines
+94
to
+97
| const { import_id } = await startResumeImport(file); | ||
| setImportId(import_id); | ||
| setPhase("polling"); | ||
| startPolling(); |
There was a problem hiding this comment.
Fix race condition: importId state may be stale when polling starts.
The setImportId(import_id) call at line 95 is asynchronous and doesn't immediately update the state. When startPolling() is called at line 97, the checkStatus callback (line 35-38) reads the importId state, which is likely still null because React hasn't re-rendered yet. This causes the "import_id が未設定です" error.
Use a ref instead of state for importId to avoid closure/stale-state issues:
🔧 Proposed fix using useRef
+import { useCallback, useRef, useState } from "react";
export function useResumeImport(): UseResumeImportReturn {
const [phase, setPhase] = useState<ResumeImportPhase>("idle");
const [parsedData, setParsedData] = useState<ResumeImportResultResponse | null>(null);
const [error, setError] = useState<AppErrorState | null>(null);
- const [importId, setImportId] = useState<string | null>(null);
+ const importIdRef = useRef<string | null>(null);
const { startPolling } = useTaskPolling({
checkStatus: async () => {
- if (!importId) throw new Error("import_id が未設定です");
- return getResumeImportStatus(importId);
+ if (!importIdRef.current) throw new Error("import_id が未設定です");
+ return getResumeImportStatus(importIdRef.current);
},
onCompleted: async () => {
- if (!importId) return;
+ if (!importIdRef.current) return;
try {
- const result = await getResumeImportResult(importId);
+ const result = await getResumeImportResult(importIdRef.current);
setParsedData(result);
// ...Update the start function:
try {
const { import_id } = await startResumeImport(file);
- setImportId(import_id);
+ importIdRef.current = import_id;
setPhase("polling");
startPolling();Update the reset function:
const reset = useCallback(() => {
setPhase("idle");
setParsedData(null);
setError(null);
- setImportId(null);
+ importIdRef.current = null;
}, []);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/hooks/career/useResumeImport.ts` around lines 94 - 97, The
polling race is caused by setImportId being async so checkStatus (used by
startPolling) reads a stale importId; replace the importId state with a ref
(e.g., importIdRef) and update importIdRef.current = import_id inside the
start/resume logic (the code around startResumeImport and start functions), then
have checkStatus/readers reference importIdRef.current instead of the importId
state; also update reset to clear importIdRef.current and remove or keep a
minimal state-only representation for UI if needed so startPolling always sees
the current import id.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/hooks/career/useResumeImport.ts (1)
41-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix stale
importIdread before polling starts.Line 100 updates state asynchronously, but Line 102 can start polling before
checkStatussees the new value, so Line 41 may throw on first poll.Proposed fix
-import { useCallback, useState } from "react"; +import { useCallback, useRef, useState } from "react"; @@ - const [importId, setImportId] = useState<string | null>(null); + const importIdRef = useRef<string | null>(null); @@ checkStatus: async () => { - if (!importId) throw new Error(INTERNAL_MESSAGES.RESUME_IMPORT_NO_ID); - return getResumeImportStatus(importId); + if (!importIdRef.current) throw new Error(INTERNAL_MESSAGES.RESUME_IMPORT_NO_ID); + return getResumeImportStatus(importIdRef.current); }, onCompleted: async () => { - if (!importId) return; + if (!importIdRef.current) return; try { - const result = await getResumeImportResult(importId); + const result = await getResumeImportResult(importIdRef.current); @@ - setImportId(import_id); + importIdRef.current = import_id; setPhase("polling"); startPolling(); @@ - setImportId(null); + importIdRef.current = null;Also applies to: 99-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/career/useResumeImport.ts` around lines 41 - 42, The polling routine reads importId from closure which may be stale because the state update at line ~100 is async, causing getResumeImportStatus(importId) to throw; fix by ensuring polling uses the committed importId value instead of the closed-over state—either capture the new importId and pass it explicitly into checkStatus/getResumeImportStatus when starting the poll (e.g., change checkStatus to accept an importId parameter and call getResumeImportStatus(passedImportId)), or delay starting the poll until the stateful importId is set (e.g., gate the poll start on importId truthiness in the effect). Update references to importId in useResumeImport.ts and adjust checkStatus/getResumeImportStatus calls accordingly.
🧹 Nitpick comments (1)
frontend/eslint.config.js (1)
46-46: ⚡ Quick winESLint exemption glob excludes
*.spec.tsx, but the repo has none
No*.spec.tsxfiles exist in this repo (search underfrontend/srcand globally returned none), so the omission won’t currently cause unexpected linting.Optional future-proof patch
- files: ["src/**/*.test.ts", "src/**/*.test.tsx", "src/**/*.spec.ts", "src/test/**/*.{ts,tsx}"], + files: [ + "src/**/*.test.ts", + "src/**/*.test.tsx", + "src/**/*.spec.ts", + "src/**/*.spec.tsx", + "src/test/**/*.{ts,tsx}", + ],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/eslint.config.js` at line 46, The ESLint config's files array currently lists "src/**/*.spec.ts" but omits spec files with .tsx extension; update the files array in eslint.config.js (the files property) to include .spec.tsx — either add "src/**/*.spec.tsx" or replace "src/**/*.spec.ts" with a combined pattern like "src/**/*.spec.{ts,tsx}" (or adjust "src/test/**/*.{ts,tsx}") so .spec.tsx files are covered by ESLint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/lint-frontend-messages.sh`:
- Around line 14-16: The comment above the PATTERN variable is inaccurate: it
says "シングルクォート文字列とバッククォート文字列" but the regex character class ["`] matches
double-quote and backtick, not single-quote; either change the comment to
"ダブルクォート文字列とバッククォート文字列" to match the existing PATTERN or modify the PATTERN from
["`] to ['\`] so it actually matches single-quote and backtick; keep the PATTERN
identifier and the Unicode property usage (\p{Hiragana}\p{Katakana}\p{Han})
unchanged and note that ShellCheck SC2016 is a false positive for these PCRE2
properties.
---
Duplicate comments:
In `@frontend/src/hooks/career/useResumeImport.ts`:
- Around line 41-42: The polling routine reads importId from closure which may
be stale because the state update at line ~100 is async, causing
getResumeImportStatus(importId) to throw; fix by ensuring polling uses the
committed importId value instead of the closed-over state—either capture the new
importId and pass it explicitly into checkStatus/getResumeImportStatus when
starting the poll (e.g., change checkStatus to accept an importId parameter and
call getResumeImportStatus(passedImportId)), or delay starting the poll until
the stateful importId is set (e.g., gate the poll start on importId truthiness
in the effect). Update references to importId in useResumeImport.ts and adjust
checkStatus/getResumeImportStatus calls accordingly.
---
Nitpick comments:
In `@frontend/eslint.config.js`:
- Line 46: The ESLint config's files array currently lists "src/**/*.spec.ts"
but omits spec files with .tsx extension; update the files array in
eslint.config.js (the files property) to include .spec.tsx — either add
"src/**/*.spec.tsx" or replace "src/**/*.spec.ts" with a combined pattern like
"src/**/*.spec.{ts,tsx}" (or adjust "src/test/**/*.{ts,tsx}") so .spec.tsx files
are covered by ESLint.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: baa98c7a-0297-476a-9c22-5522448ef2ed
📒 Files selected for processing (25)
.claude/CLAUDE.md.claude/rules/frontend/messages.mdMakefilebackend/app/routers/resume_imports.pybackend/app/services/resume_import/llm_extractor.pybackend/app/services/resume_import/pdf_extractor.pybackend/app/services/resume_import/prompts/extract_resume.mdbackend/app/services/tasks/handlers/resume_import.pybackend/requirements.txtfrontend/eslint.config.jsfrontend/src/api/auth.tsfrontend/src/api/client.tsfrontend/src/api/download.tsfrontend/src/components/ErrorBoundary.tsxfrontend/src/components/forms/CareerResumeForm.tsxfrontend/src/constants/messages.tsfrontend/src/formMappers.tsfrontend/src/hooks/blog/useBlogAccountManager.tsfrontend/src/hooks/blog/useBlogSummaryPolling.tsfrontend/src/hooks/career/useCareerAnalysisPage.tsfrontend/src/hooks/career/useResumeImport.tsfrontend/src/hooks/useDocumentForm.tsfrontend/src/hooks/usePdfActions.tsfrontend/src/payloadBuilders.tsscripts/lint-frontend-messages.sh
✅ Files skipped from review due to trivial changes (3)
- frontend/src/api/download.ts
- .claude/CLAUDE.md
- backend/app/services/resume_import/prompts/extract_resume.md
Owner
Author
frontend: エラーメッセージのハードコード除去と再発防止機構の追加Summary
Context調査でユーザー認識と実態にズレが判明した:
このまま放置すると、ユーザー向け文言の修正・i18n 化・レビューの抜けが発生しやすい。設計判断として「 変更点Frontend（メッセージ集約）新規ファイル:
|
| 定数名 | 用途 | 件数 |
|---|---|---|
VALIDATION_MESSAGES |
フォーム事前バリデーション | 9 |
NETWORK_MESSAGES |
API クライアント層の fallback | 4 |
FALLBACK_MESSAGES |
catch ブロック / toAppError fallback |
22 |
UI_MESSAGES |
JSX 直書き文言（ErrorBoundary 等） | 2 |
INTERNAL_MESSAGES |
開発者向け内部エラー | 1 |
downloadFailureMessage(filename) |
動的パラメータが必要な関数版 | - |
置換ファイル（38 件 → 0 件）
| ファイル | 件数 | 主な置換先 |
|---|---|---|
payloadBuilders.ts |
8 | VALIDATION_MESSAGES.* |
api/client.ts |
4 | NETWORK_MESSAGES.* |
api/download.ts |
2 | FALLBACK_MESSAGES.DOWNLOAD / .PREVIEW_FETCH / downloadFailureMessage() |
api/auth.ts |
2 | FALLBACK_MESSAGES.AUTH_CHECK / .GITHUB_OAUTH_START |
hooks/usePdfActions.ts |
3 | FALLBACK_MESSAGES.PDF_DOWNLOAD / .MARKDOWN_DOWNLOAD / .PREVIEW |
hooks/useDocumentForm.ts |
2 | FALLBACK_MESSAGES.SAVE / .DELETE |
hooks/career/useResumeImport.ts |
4 | VALIDATION_MESSAGES.RESUME_PDF_* / FALLBACK_MESSAGES.RESUME_EXTRACT / INTERNAL_MESSAGES.RESUME_IMPORT_NO_ID |
hooks/career/useCareerAnalysisPage.ts |
3 | FALLBACK_MESSAGES.ANALYSIS* |
hooks/blog/useBlogAccountManager.ts |
6 | FALLBACK_MESSAGES.BLOG_* |
hooks/blog/useBlogSummaryPolling.ts |
3 | FALLBACK_MESSAGES.BLOG_SUMMARY_*（grep 検知で追加発見） |
components/ErrorBoundary.tsx |
2 | UI_MESSAGES.ERROR_BOUNDARY_* |
| 合計 | 38 |
対象外:
pages/GitHubCallbackPage.tsxの URL クエリ (invalid_callback等) はエラーコードでありメッセージではないhooks/useAuthSession.ts:115のconsole.warnは UI に出ない開発者ログ
再発防止機構（4 層）
1. ESLint: no-restricted-syntax
frontend/eslint.config.js に追加。Unicode プロパティで日本語（ひらがな・カタカナ・漢字）を含むリテラルのみを抑止し、英語の開発者向けエラーは許容する。
"no-restricted-syntax": [
"error",
{
selector:
"ThrowStatement > NewExpression[callee.name='Error'] > Literal[value=/[\\u3040-\\u309F\\u30A0-\\u30FF\\u4E00-\\u9FAF]/]",
message: "throw new Error にリテラル日本語を直接書かない。frontend/src/constants/messages.ts の定数を参照すること。",
},
{
selector:
"ThrowStatement > NewExpression[callee.name='Error'] > TemplateLiteral:has(TemplateElement[value.raw=/[\\u3040-\\u309F\\u30A0-\\u30FF\\u4E00-\\u9FAF]/])",
message: "throw new Error にテンプレートリテラルで日本語を直接書かない。...",
},
],テストファイル（*.test.ts, *.spec.ts, src/test/**）はフィクスチャ throw を許容するため override で off にしている。
2. Makefile + shell script: make lint-frontend-messages
ESLint AST では拾いきれない関数呼び出し系（setError("...") / setSummaryError("...") / toast.error("...") / alert("...")）を scripts/lint-frontend-messages.sh の ripgrep \p{Hiragana}\p{Katakana}\p{Han} パターンで補完。make lint ターゲットに組み込まれているため CI で自動的に走る。
3. AI エージェント向け運用ルール: .claude/rules/frontend/messages.md
frontend を編集するときに自動ロードされる場所に、SSoT の責務分離・新規メッセージ追加手順・やってはいけないこと・正しい書き方を明文化。
4. 共通規約への追記: .claude/CLAUDE.md
「コーディング規約（共通）」セクションに「エラーメッセージのハードコード禁止」ルールを追記し、詳細リンクを .claude/rules/frontend/messages.md に集約。
Test plan
-
make lint-frontend— ESLint pass（新ルール含む） -
make lint-frontend-messages— grep 検知 pass（残存 0 件） -
make build-frontend— tsc + vite ビルド成功 -
make test-frontend— node:test 4 件 + vitest 148 件すべて pass - ESLint ルールの自己検証: 一時的に違反コード (
throw new Error("これはテスト用の違反です")) を入れてmake lint-frontendが error で fail することを確認 → クリーンアップ済み - grep 検知の自己検証: 初回実行で
useBlogSummaryPolling.tsの漏れ 3 件を検知 → 追加修正済み（仕組みが意図通り効いている証拠）
動作確認の推奨手順（レビュアー向け）
ブラウザでの目視確認はレビュー時に以下を推奨:
- 職務経歴フォームの「氏名未入力で保存」→
VALIDATION_MESSAGES.FULL_NAME_REQUIREDが画面に出ること - 開発サーバ停止状態で API を呼ぶ →
NETWORK_MESSAGES.CONNECTION_FAILEDが ErrorToast に出ること - ErrorBoundary を強制発火 →
UI_MESSAGES.ERROR_BOUNDARY_*が出ること
E2E トリガー条件（新規ページ・認証フロー・レイアウト変更）には該当しないため、npm run test:e2e は変更内容に対しては影響なし（既存 E2E が通れば OK）。
影響範囲
新規:
frontend/src/constants/messages.tsscripts/lint-frontend-messages.sh.claude/rules/frontend/messages.md
編集:
frontend/src/payloadBuilders.tsfrontend/src/api/client.tsdownload.tsauth.tsfrontend/src/hooks/usePdfActions.tsuseDocumentForm.tsfrontend/src/hooks/career/useResumeImport.tsuseCareerAnalysisPage.tsfrontend/src/hooks/blog/useBlogAccountManager.tsuseBlogSummaryPolling.tsfrontend/src/components/ErrorBoundary.tsxfrontend/eslint.config.jsMakefile.claude/CLAUDE.md
変更行数: 約 +250 / -90（うち新規定数ファイルが約 75 行、ルール docs が約 90 行、置換は 38 箇所）
やらないこと（スコープ外）
messages.jsonから TS 定数を build-time 生成するパイプライン（既存 ERROR_CONFIG が手動同期で機能しているため投資対効果が低い）- i18n（i18next 等）の導入（現状すべて日本語のため不要）
GitHubCallbackPage.tsxの OAuth エラーコード定数化（コードであってメッセージではない）- backend 側の
messages.py/messages.json構造の改修 ErrorCodeenum とmessages.jsonの同期チェック CI（別タスク）
関連
- 詳細設計プラン:
/Users/wadayuusuke/.claude/plans/ts-tsx-users-wadayuusuke-documents-dev-d-warm-squid.md - 既存の関連ファイル:
frontend/src/constants/errorCodes.tserrorMessages.ts/backend/app/core/errors.pymessages.json
🤖 Generated with Claude Code
This was referenced May 28, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat: 職務経歴書フォームに未保存変更インジケーター（Dirty Dot）を追加
概要
職務経歴書フォームの各セクション・フィールドに、未保存の変更があることを示す赤丸マーク（Dirty Dot）を表示する機能を実装した。VS Code のファイル変更マークと同様の UX を目指し、編集中のどの箇所が未保存なのかを一目で把握できるようにする。
変更内容
新規追加
frontend/src/components/ui/DirtyDot.tsxfrontend/src/components/ui/DirtyDot.module.cssfrontend/src/hooks/career/useCareerDirty.tsfrontend/src/hooks/career/useCareerDirty.test.tsfrontend/src/hooks/career/useProjectFormDirty.tsfrontend/src/hooks/career/useProjectFormDirty.test.tsfrontend/e2e/career-dirty-indicator.spec.ts既存ファイル変更
frontend/src/hooks/useDocumentForm.tsbaselinestate を追加（サーバ最新スナップショット。load 成功・save 成功時のみ更新）commitBaseline()ヘルパーを追加（ローカル state と Redux キャッシュを同時更新）setForm内のdispatchをqueueMicrotaskで render phase の外に逃がし、Cannot update a component while rendering警告を解消documentIdRefを追加し、setCache 発行時に正確な documentId を書き込むuseDocumentFormの戻り値にbaselineを追加frontend/src/store/formCacheSlice.tsFormCacheEntryにbaselineフィールドを追加setBaselineアクションを追加（サーバ同期完了時専用）setCacheでキャッシュ上書き時に既存 baseline を保持するよう修正frontend/src/components/forms/CareerResumeForm.tsx/ 各 Section コンポーネントuseCareerDirtyを呼び出し、各見出し・フィールド横に<DirtyDot>を配置frontend/src/components/forms/ProjectModal.tsxuseProjectFormDirtyを呼び出し、モーダルタイトル・各セクション横に<DirtyDot>を配置backend/app/schemas/resume.py/backend/tests/test_schemas.py設計メモ
baselineは ロード完了・保存完了のタイミングでのみ更新 する。ユーザーの編集中（setForm）では更新しない。これにより「サーバに保存済みの状態との差分」を常に正確に算出できる。baseline === null（未ロード）のときは dirty マップをすべてfalseとし、誤検出を防ぐ。DirtyDotは CSS で赤丸を描画しており、絵文字の光沢グラデーション等のフォント依存の見た目ブレを回避している。テスト計画
useCareerDirty単体テスト（追加済み）useProjectFormDirty単体テスト（追加済み）🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation