Develop To Main#88
Merged
Merged
Conversation
feat: フロントページのスタイルを更新
chore: 管理画面でアニメーション等が当たらないよう実装を見直し
feat: カラートークンの整理
feat: ロギング設計
feat: miseツールに gotestsum を追加
chore: biomeのリント設定に複雑度を厳格に見るよう指定
feat: APIのグレースフルシャットダウン機能
chore: slog-lintを導入
refactor: 管理画面コード
Contributor
Walkthroughバックエンドにobsベースの観測スタック（ログ/トレース/メトリクス）を導入し、既存のslog初期化/リクエストロギングを置換。フロントエンドはLitコンテキストとRepositoryパターンへ移行し、多数のフォーム/UIコンポーネントとテーマトークンを追加・再編しました。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 30
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/utils/scroll.ts (1)
62-69:⚠️ Potential issue | 🟡 MinorJSDocコメントが実装と不整合です。
Line 62のコメントでは
#f3f2ee →#ffffffと記載されていますが、実際の実装は `#0d0d0c → `#161514のダークカラーに変更されています。コメントを更新してください。📝 コメント修正案
/** - * スクロール量に応じて body の背景色を `#f3f2ee` → `#ffffff` へ補間。 + * スクロール量に応じて body の背景色を `#0d0d0c` → `#161514` へ補間。 * クリーンアップ関数を返す。 */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/utils/scroll.ts` around lines 62 - 69, Update the JSDoc for setupBackgroundShift to match the actual colors used: replace the described gradient "#f3f2ee → `#ffffff`" with the implemented color values "#0d0d0c → `#161514`" (or mention the hex equivalents of the from and to objects) so the comment reflects the implementation using the from and to color objects inside setupBackgroundShift.frontend/src/pages/page-top.ts (1)
176-188:⚠️ Potential issue | 🟠 Major記事プレビューのリンクもスキーム検証を入れてください。
プロフィールの
linksはサニタイズされていますが、ここはarticle.urlをそのままhrefに入れています。管理画面経由で危険な URL が保存されるとトップページからそのまま踏めてしまうので、こちらもhttp(s)のみ許可する検証を通すべきです。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/page-top.ts` around lines 176 - 188, The article link currently inserts article.url directly into the href in the articles.map render, which allows non-http(s) schemes; update the rendering to validate/sanitize article.url before use: create or reuse a sanitizer function (e.g., validateArticleUrl or integrate into formatArticleDate flow) that uses the URL constructor in a try/catch to ensure the protocol is either "http:" or "https:" and returns either the original URL, a safe fallback (e.g., "#" or a routed internal preview), or null; in the map rendering (the block that calls this.formatArticleDate and uses article.url) use that validated URL for href and skip or render a non-clickable element when validation fails to prevent unsafe schemes from being injected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/cmd/api/main.go`:
- Around line 63-66: The defer uses a hardcoded 5s fallback which mismatches the
previously defined shutdownTimeout (30*time.Second); replace the hardcoded
5*time.Second with the existing shutdownTimeout constant (or introduce and use a
clearly named constant like shortShutdownTimeout if a shorter value is
intentional) and update the defer block that assigns shutdownCtx and
shutdownCancel (referencing shutdownCtx, shutdownCancel, shutdownTimeout) or add
a brief comment explaining why a different fallback is used.
In `@backend/cmd/batch/local.go`:
- Around line 39-43: The deferred shutdown call currently ignores the error from
shutdown(shutdownCtx); change the defer body in the anonymous func to capture
the returned error (e.g., err := shutdown(shutdownCtx)) and, if err != nil, log
it so flush/finalization failures for tracer/meter are visible; keep the
existing context cancel() defer and use your project's logger (or log.Printf) to
emit a clear message referencing shutdown and shutdownCtx when err != nil.
- Around line 45-48: The defer call to obs.RecoverProcess in
backend/cmd/batch/local.go won't catch panics because recover must be called
from the deferred function itself; wrap the call in an anonymous function (e.g.,
defer func(){ obs.RecoverProcess(ctx, "batch.main") }() ) so RecoverProcess runs
inside a deferred closure that can call recover, or alternatively change the
RecoverProcess API to return a closure (e.g., func RecoverProcess(...) func())
and defer the returned function; update any callers accordingly.
In `@backend/internal/domain/identity/entity.go`:
- Around line 199-209: The IsActive method uses time.Now() directly and compares
status via Value(), which makes testing/time control hard and is verbose; change
Session.IsActive to accept a time parameter (e.g., now time.Time) or use an
injected clock interface and use that instead of time.Now(), and simplify the
status check from e.status.Value() == statusActive.Value() to a direct
comparison e.status == statusActive; ensure IsRevoked uses the same
direct-status comparison pattern (e.status == statusRevoked) for consistency.
In `@backend/pkg/obs/attr.go`:
- Around line 17-18: Update the comment above the attribute constants to
explicitly state that AttrTraceID and AttrSpanID (in addition to request.id) are
custom attributes that deviate from OpenTelemetry semantic conventions (which
use dot-form keys like "trace.id" and "span.id"); locate the constants
AttrTraceID and AttrSpanID and amend the surrounding comment to mention these
are non-standard/extension keys and differ from the OTel sem-conv names for
clarity.
In `@backend/pkg/obs/obstest/capture.go`:
- Around line 47-61: Records currently swallows JSON decode errors inside
Capture.Records, causing tests to miss corrupted logs; change the API to return
([]map[string]any, error) from Capture.Records (or alternatively have it call
testing.T.Fatalf if you prefer immediate test failure) and propagate the decoder
error instead of returning partial results—replace the current dec.Decode error
branch to return nil, err (or call t.Fatalf with the error) and update callers
to handle the returned error accordingly.
In `@backend/pkg/obs/recover.go`:
- Around line 14-29: The current RecoverProcess calls recover() inside a regular
function so it never captures panics when used as defer; change RecoverProcess
to return a closure (signature: RecoverProcess(ctx context.Context, op string)
func()) that performs the recover(), logging (slog.ErrorContext with AttrOp,
AttrExceptionMessage, AttrExceptionStack) and then re-panics, and update all
call sites to use defer obs.RecoverProcess(ctx, "batch.main")() so the recover
runs directly in the deferred function and actually captures panics.
In `@frontend/biome.json`:
- Around line 30-33: Update the lint rule severity under the "style" object by
changing the "noParameterAssign" setting from "warn" to "error" so parameter
reassignment is enforced as an error; locate the "style" block (contains
"noNonNullAssertion" and "noParameterAssign") and modify the "noParameterAssign"
value accordingly.
In `@frontend/src/components/admin/profile/me-profile-certifications-editor.ts`:
- Around line 111-115: The change handler for the year input currently forces
Number(e.detail || '0') which converts empty input to 0; update the handler in
the me-text-input change callback (the lambda that calls this.updateItem) to
avoid mapping empty strings to 0 — instead detect empty/null/undefined e.detail
and pass undefined or a fallback (e.g., new Date().getFullYear()) to
this.updateItem so the stored year mirrors the month-field behavior; locate the
inline handler where updateItem is called and replace the Number(e.detail ||
'0') logic with a conditional that returns undefined or the current year when
e.detail is empty, otherwise Number(e.detail).
In `@frontend/src/components/admin/profile/me-profile-experiences-editor.ts`:
- Around line 112-116: The startYear input currently converts empty input to 0
using Number(e.detail || '0'); update this to mirror the endYear handling or use
a sensible fallback (e.g., current year) instead of 0: detect empty or falsy
e.detail and set startYear to null or to new Date().getFullYear() (or the same
fallback used for endYear) before calling this.updateItem(index, { startYear:
... }); update the handler attached to the me-text-input change event so it uses
the same empty-value logic as endYear and ensure updateItem receives the
corrected value.
In `@frontend/src/components/admin/profile/me-profile-links-editor.ts`:
- Around line 23-40: dispatchChange and updated both call
this._internals.setFormValue causing redundant calls when links change
internally; modify updated to skip calling setFormValue when the links value did
not actually change by comparing the previous value from
changedProperties.get('links') (or JSON.stringify of it) with the current
this.links (or its JSON string) and only call
this._internals.setFormValue(JSON.stringify(this.links ?? [])) when they differ;
keep dispatchChange as-is so user-driven updates still set the form value and
emit the change event.
In `@frontend/src/components/admin/ui/me-select.ts`:
- Line 13: required プロパティの変更が ElementInternals
に反映されておらずフォーム検証が正しく動作しないので、カスタム要素（MeSelect クラス）で ElementInternals
を取得（attachInternals）し、required または value が変更されるたびに
this.internals.setValidity(...) を呼んで妥当性を同期してください。具体的には、required が true かつ value
が空のときは valueMissing エラーを渡して setValidity({ valueMissing: true }, "必須項目です", ネイティブ
input/select 要素)、value が存在するまたは required が false のときは
this.internals.setValidity({}) でクリアする処理を、プロパティ反映/updated フック（Lit の
updated/attributeChangedCallback 等）に追加してください。
- Around line 55-59: When this.label is falsy the rendered <select> lacks an
accessible name; update the component rendering (the template that uses
this.label and this._inputId in me-select.ts) to provide a fallback accessible
name by adding either an aria-label (e.g. this.ariaLabel or a default string) or
aria-labelledby that references an offscreen/visually-hidden element; ensure the
fallback is applied only when this.label is not present and that the select
element includes the attribute (e.g. aria-label=${this.label ??
this._fallbackLabel} or aria-labelledby=${this._inputIdFallback}) so assistive
tech can always announce the control.
- Around line 23-26: formResetCallback currently always resets the control to ''
which violates the Form Association spec; change it to restore the element's
initial value instead: capture and store the initial value (e.g. in the
constructor or connectedCallback as this._initialValue using
this.hasAttribute('value') ? this.getAttribute('value') : '' or the current
this.value) and then update formResetCallback to set this.value =
this._initialValue and call this._internals.setFormValue(this._initialValue) so
resets restore the original attribute-initialized value rather than always
clearing.
- Line 10: The host element's name property is not reflected to its attribute so
form-associated submission omits the key; change the property declaration in
MeSelect (the `@property`() name = '') to reflect to the host (e.g., use
`@property`({ reflect: true }) for name) or otherwise sync the property to the
host attribute when it changes so the host element has a name attribute and the
form includes the field key on submit.
In `@frontend/src/components/admin/ui/me-text-input.ts`:
- Around line 177-179: The CSS rule using input:invalid:not(:placeholder-shown)
is brittle when placeholder is absent; update the me-text-input component to
avoid :placeholder-shown — either ensure a placeholder is always provided by the
MeTextInput component or change the selector to rely on focus/explicit empty
state (e.g. use input:invalid:focus to show validation only when the field is
focused, or implement a data attribute like [data-empty="true"] set by
MeTextInput and use input:invalid:not([data-empty="true"])). Locate the selector
in frontend/src/components/admin/ui/me-text-input.ts and replace the
:placeholder-shown dependency accordingly.
In `@frontend/src/components/admin/ui/me-textarea.ts`:
- Around line 31-34: formResetCallback currently forces this.value to '' instead
of restoring the element's initial value; change it to restore a stored initial
value: capture the initial value when the element is constructed/connected
(e.g., set this._initialValue = this.hasAttribute('value') ?
this.getAttribute('value') : this.value or set in
connectedCallback/constructor), and then update formResetCallback to set
this.value = this._initialValue and
this._internals.setFormValue(this._initialValue); ensure the initial capture
uses the same symbol names (this._initialValue, formResetCallback,
this._internals.setFormValue) so reset returns the element to its original value
rather than an empty string.
- Around line 62-66: updated() currently only watches 'value' changes so
toggling required or disabled doesn't call _syncInternals(), causing
ElementInternals validity to go out of sync; update the changedProperties check
in the updated method (the updated function) to also detect changes to
'required' and 'disabled' and call this._syncInternals() when any of those three
properties changed, ensuring _syncInternals() runs after changes to value,
required, or disabled so checkValidity() remains consistent with the input
state.
- Around line 48-60: The component currently emits a CustomEvent named "change"
on every keystroke from _onInput, breaking HTML semantics and causing parents
like me-profile-skills-editor to react too eagerly; change _onInput to only
update this.value and call _syncInternals and then dispatch an "input" event (or
no custom event) for per-keystroke updates, and add a blur/commit handler (e.g.,
_onBlur or when edit is committed) that dispatches the CustomEvent("change", {
detail: this.value, bubbles: true, composed: true }) so parents receive the
standard "change" only on focus loss/commit; keep internal calls to
_syncInternals and ensure the handler names (_onInput, _onBlur) are wired in the
template.
In `@frontend/src/components/app-root.ts`:
- Around line 148-161: firstUpdated と currentPath
変更時のみでなく認証状態変化でも管理ルート同期を走らせる必要があります: protected updated(changedProperties) 内に
changedProperties.has('auth') チェックを追加し、const prevAuth =
changedProperties.get('auth') として prevAuth?.status と this.auth.status
を比較し異なれば（かつ this.isAdminPath(this.currentPath) が真なら） void
this.syncAdminRouteState() を呼ぶようにしてください; 参照する識別子は firstUpdated, updated,
syncAdminRouteState, auth.status, currentPath, isAdminPath, login,
refreshSession です。
In `@frontend/src/domain/ProfileRepository.ts`:
- Around line 101-110: The in-flight fetch in _internalFetch() can overwrite
newer state when saveAdminProfile() completes first; fix by adding an
invalidation/guard: either have saveAdminProfile() clear the shared
_fetchPromise or bump a fetchVersion token before starting save so that
_internalFetch() checks the token (or fetchId) before assigning to
_publicProfile/_adminProfile and only applies results when the token matches;
update both _internalFetch() and the saveAdminProfile() success path (and the
analogous code block referenced at the other location) to use this
invalidate-or-guard pattern so stale getMe() responses are ignored.
- Around line 101-107: The bug is that _internalFetch (and the load/save paths)
assign the same MeProfile object to both _publicProfile and _adminProfile,
causing destructive edits to adminProfile to leak into publicProfile; fix by
assigning separate clones instead of the shared reference (e.g., in
_internalFetch, when setting this._publicProfile = p and this._adminProfile = p,
replace one or both with a deep-cloned copy of p using an existing cloning
utility or structuredClone/cloneMeProfile so each state holds its own MeProfile
instance), and apply the same cloning approach in the load and save locations
that set _publicProfile/_adminProfile.
- Around line 84-91: loadAdminProfile() currently calls _internalFetch() which
populates adminProfile but doesn't mark _adminLoaded, causing duplicate getMe()
calls when moving from public to admin; update loadAdminProfile() to set
this._adminLoaded = true (and clear this._adminLoading) immediately after a
successful await this._internalFetch(), or change the initial guard to check for
actual data (e.g. if (this._adminLoaded || this._adminLoading ||
this.adminProfile) return) so that already-populated adminProfile prevents
re-fetching; apply the same change to the analogous block at lines 101-108 to
avoid redundant network calls there as well.
In `@frontend/src/domain/Repository.ts`:
- Around line 7-10: Extract the hard-coded event name 'change' into a single
constant and use that constant in notifyChange instead of inline string to avoid
duplication and typos; for example add an exported or class-level constant like
CHANGE_EVENT (referenced in Repository.notifyChange) and replace new
Event('change') with new Event(CHANGE_EVENT), and update any other places that
dispatch or listen for this event to use the same constant.
In `@frontend/src/pages/page-admin-account.ts`:
- Around line 22-26: render() currently accesses this.authRepo (backed by
private _authRepo) without guarding against undefined; add a null/undefined
check at the start of render() (or within the authRepo getter) to handle cases
before Lit provides context—e.g., if (!this.authRepo) return a safe
placeholder/template or render a loading/error state—so accesses to properties
on `_authRepo` (and the local `a` variable) cannot cause a runtime error.
In `@frontend/src/pages/page-admin-articles.ts`:
- Around line 560-566: handleReset currently only clears dirty flags
(this.localDirty, this.articleRepo.setAdminDirty) but does not restore the form
values to the saved baseline; update handleReset so that after
confirmDiscardChanges() succeeds you reinitialize the form controls from the
baseline (e.g., restore values from this.baseline into all bound fields / <me-*>
components), ensure any per-field state is reset (validation, selected options,
etc.), then clear the dirty flags and call this.requestUpdate(); if there isn't
an existing helper, add a clear/reset method (e.g., resetFormToBaseline or
initFormFromBaseline) and call it from handleReset.
- Around line 523-533: The save flow leaves the repository-level dirty flag set:
handleInput() marks this.articleRepo.adminDirty = true but the success branch
only clears this.localDirty; update the save logic (the block that calls
updateArticle/createArticle, setBaseline, Promise.all, etc.) to also reset
this.articleRepo.adminDirty = false on successful save (after setBaseline or
after Promise.all) so the repository dirty state, the leave-confirmation and
submit-button enablement are restored; ensure both the 'edit' and create
branches clear articleRepo.adminDirty and keep existing calls to
setBaseline(cloneArticleDraft(draft)), this.localDirty = false, and the
reloadArticles()/refreshTags() Promise.all.
In `@frontend/src/pages/page-admin-profile.ts`:
- Around line 211-220: handleReset currently only clears the dirty flag but does
not restore the visible form values; update the handleReset implementation (the
method named handleReset) so after confirming and calling
this.profileRepo.setAdminDirty(false) it also re-initializes the form controls
from the repository's canonical data (this.profileRepo.adminProfile) — e.g.,
assign each field/component value from this.profileRepo.adminProfile (or call a
repo method that returns a snapshot) so each <me-*> input reflects adminProfile,
then call this.requestUpdate() to re-render; ensure you reference the
this.profileRepo.adminProfile object and the handleReset method when making the
change.
In `@frontend/src/pages/page-articles.ts`:
- Around line 188-194: The tag toggle button in renderTagToggle should expose
its expanded/collapsed state to assistive tech: update the <button> rendered by
renderTagToggle to include aria-expanded="${this.showAllTags}" (and add
aria-controls referencing the id of the tag list element if one exists) so
screen readers can determine current state; keep the existing class
"tag-toggle", the click handler that flips this.showAllTags, and the current
label logic using this.tagOptions.length.
In `@mise.toml`:
- Line 5: 不要な可変指定が入っている gotestsum = "latest" を固定バージョンに差し替えて再現性を確保してください:
mise.toml 内の gotestsum エントリ（gotestsum =
"latest"）をプロジェクトで採用している他ツールと同様の厳密なバージョン文字列に置き換え、依存管理や CI
のインストールスクリプト／キャッシュがあれば合わせて更新してから CI を再実行して問題ないことを確認してください。
---
Outside diff comments:
In `@frontend/src/pages/page-top.ts`:
- Around line 176-188: The article link currently inserts article.url directly
into the href in the articles.map render, which allows non-http(s) schemes;
update the rendering to validate/sanitize article.url before use: create or
reuse a sanitizer function (e.g., validateArticleUrl or integrate into
formatArticleDate flow) that uses the URL constructor in a try/catch to ensure
the protocol is either "http:" or "https:" and returns either the original URL,
a safe fallback (e.g., "#" or a routed internal preview), or null; in the map
rendering (the block that calls this.formatArticleDate and uses article.url) use
that validated URL for href and skip or render a non-clickable element when
validation fails to prevent unsafe schemes from being injected.
In `@frontend/src/utils/scroll.ts`:
- Around line 62-69: Update the JSDoc for setupBackgroundShift to match the
actual colors used: replace the described gradient "#f3f2ee → `#ffffff`" with the
implemented color values "#0d0d0c → `#161514`" (or mention the hex equivalents of
the from and to objects) so the comment reflects the implementation using the
from and to color objects inside setupBackgroundShift.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cd1f87b1-914e-42ad-8c30-8a3a1eefc271
⛔ Files ignored due to path filters (2)
backend/go.sumis excluded by!**/*.sumpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (68)
backend/.golangci.ymlbackend/cmd/api/main.gobackend/cmd/batch/local.gobackend/go.modbackend/internal/app/article/interactor.gobackend/internal/app/identity/interactor.gobackend/internal/app/me/interactor.gobackend/internal/domain/identity/entity.gobackend/internal/domain/identity/entity_test.gobackend/internal/handler/article/handler.gobackend/internal/handler/identity/handler.gobackend/internal/handler/me/handler.gobackend/pkg/errs/http.gobackend/pkg/errs/internal.gobackend/pkg/middleware/logging.gobackend/pkg/middleware/logging_test.gobackend/pkg/middleware/recover.gobackend/pkg/middleware/recover_test.gobackend/pkg/middleware/requestid.gobackend/pkg/obs/attr.gobackend/pkg/obs/context.gobackend/pkg/obs/log.gobackend/pkg/obs/logger.gobackend/pkg/obs/meter.gobackend/pkg/obs/obs.gobackend/pkg/obs/obstest/capture.gobackend/pkg/obs/recover.gobackend/pkg/obs/redact.gobackend/pkg/obs/tracer.gobackend/pkg/slogx/slogx.gofrontend/biome.jsonfrontend/package.jsonfrontend/src/admin/admin-form-styles.tsfrontend/src/components/admin/profile/me-profile-certifications-editor.tsfrontend/src/components/admin/profile/me-profile-experiences-editor.tsfrontend/src/components/admin/profile/me-profile-links-editor.tsfrontend/src/components/admin/profile/me-profile-skills-editor.tsfrontend/src/components/admin/ui/me-admin-panel.tsfrontend/src/components/admin/ui/me-admin-section.tsfrontend/src/components/admin/ui/me-select.tsfrontend/src/components/admin/ui/me-text-input.tsfrontend/src/components/admin/ui/me-textarea.tsfrontend/src/components/app-admin-shell.tsfrontend/src/components/app-root.tsfrontend/src/contexts/article-context.tsfrontend/src/contexts/auth-context.tsfrontend/src/contexts/profile-context.tsfrontend/src/controllers/RepositoryObserver.tsfrontend/src/domain/ArticleRepository.tsfrontend/src/domain/AuthRepository.tsfrontend/src/domain/ProfileRepository.tsfrontend/src/domain/Repository.tsfrontend/src/index.cssfrontend/src/pages/page-about.tsfrontend/src/pages/page-admin-account.tsfrontend/src/pages/page-admin-articles.tsfrontend/src/pages/page-admin-login.tsfrontend/src/pages/page-admin-profile.tsfrontend/src/pages/page-articles.tsfrontend/src/pages/page-top.tsfrontend/src/styles/theme-admin.cssfrontend/src/styles/theme-public.cssfrontend/src/styles/tokens.cssfrontend/src/tokens.cssfrontend/src/utils/ambient.tsfrontend/src/utils/cursor.tsfrontend/src/utils/scroll.tsmise.toml
💤 Files with no reviewable changes (5)
- backend/pkg/errs/http.go
- backend/pkg/middleware/logging.go
- backend/pkg/middleware/logging_test.go
- backend/pkg/slogx/slogx.go
- frontend/src/tokens.css
Comment on lines
+63
to
+66
| defer func() { | ||
| if shutdownCtx == nil { | ||
| shutdownCtx, shutdownCancel = context.WithTimeout(context.Background(), 5*time.Second) | ||
| } |
Contributor
There was a problem hiding this comment.
シャットダウンタイムアウトの不整合
Line 31 で shutdownTimeout = 30 * time.Second と定義されていますが、Line 65 のフォールバックでは 5*time.Second がハードコードされています。一貫性のため、定数を使用するか、フォールバック用に別の定数を定義することを推奨します。
🐛 修正案
if shutdownCtx == nil {
- shutdownCtx, shutdownCancel = context.WithTimeout(context.Background(), 5*time.Second)
+ shutdownCtx, shutdownCancel = context.WithTimeout(context.Background(), shutdownTimeout)
}または、早期終了時の短いタイムアウトが意図的であれば、コメントでその理由を説明してください。
📝 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
| defer func() { | |
| if shutdownCtx == nil { | |
| shutdownCtx, shutdownCancel = context.WithTimeout(context.Background(), 5*time.Second) | |
| } | |
| defer func() { | |
| if shutdownCtx == nil { | |
| shutdownCtx, shutdownCancel = context.WithTimeout(context.Background(), shutdownTimeout) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/cmd/api/main.go` around lines 63 - 66, The defer uses a hardcoded 5s
fallback which mismatches the previously defined shutdownTimeout
(30*time.Second); replace the hardcoded 5*time.Second with the existing
shutdownTimeout constant (or introduce and use a clearly named constant like
shortShutdownTimeout if a shorter value is intentional) and update the defer
block that assigns shutdownCtx and shutdownCancel (referencing shutdownCtx,
shutdownCancel, shutdownTimeout) or add a brief comment explaining why a
different fallback is used.
Comment on lines
+39
to
+43
| defer func() { | ||
| shutdownCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| defer cancel() | ||
| _ = shutdown(shutdownCtx) | ||
| }() |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
シャットダウンエラーのログ出力を検討
shutdown(shutdownCtx) のエラーを _ で無視していますが、tracer/meter の flush 失敗時にログを残すと、観測性データの欠損を把握しやすくなります。
♻️ エラーログ追加案
defer func() {
shutdownCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
- _ = shutdown(shutdownCtx)
+ if err := shutdown(shutdownCtx); err != nil {
+ slog.Error("observability shutdown failed", "error", err)
+ }
}()📝 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
| defer func() { | |
| shutdownCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | |
| defer cancel() | |
| _ = shutdown(shutdownCtx) | |
| }() | |
| defer func() { | |
| shutdownCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | |
| defer cancel() | |
| if err := shutdown(shutdownCtx); err != nil { | |
| slog.Error("observability shutdown failed", "error", err) | |
| } | |
| }() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/cmd/batch/local.go` around lines 39 - 43, The deferred shutdown call
currently ignores the error from shutdown(shutdownCtx); change the defer body in
the anonymous func to capture the returned error (e.g., err :=
shutdown(shutdownCtx)) and, if err != nil, log it so flush/finalization failures
for tracer/meter are visible; keep the existing context cancel() defer and use
your project's logger (or log.Printf) to emit a clear message referencing
shutdown and shutdownCtx when err != nil.
Comment on lines
+45
to
+48
| // RecoverProcess はログ出力後に re-panic するので、batch のプロセス終了は | ||
| // 非ゼロ exit になる (runtime のデフォルト panic ハンドラが exit 2 を返す)。 | ||
| defer obs.RecoverProcess(ctx, "batch.main") | ||
|
|
Contributor
There was a problem hiding this comment.
defer obs.RecoverProcess が期待通りに動作しない
recover.go で指摘した通り、この defer パターンでは recover() がパニックを捕捉できません。正しく動作させるには無名関数でラップする必要があります。
🐛 修正案
- defer obs.RecoverProcess(ctx, "batch.main")
+ defer func() {
+ obs.RecoverProcess(ctx, "batch.main")
+ }()または RecoverProcess の設計を変更し、クロージャを返すようにしてください。
📝 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
| // RecoverProcess はログ出力後に re-panic するので、batch のプロセス終了は | |
| // 非ゼロ exit になる (runtime のデフォルト panic ハンドラが exit 2 を返す)。 | |
| defer obs.RecoverProcess(ctx, "batch.main") | |
| // RecoverProcess はログ出力後に re-panic するので、batch のプロセス終了は | |
| // 非ゼロ exit になる (runtime のデフォルト panic ハンドラが exit 2 を返す)。 | |
| defer func() { | |
| obs.RecoverProcess(ctx, "batch.main") | |
| }() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/cmd/batch/local.go` around lines 45 - 48, The defer call to
obs.RecoverProcess in backend/cmd/batch/local.go won't catch panics because
recover must be called from the deferred function itself; wrap the call in an
anonymous function (e.g., defer func(){ obs.RecoverProcess(ctx, "batch.main")
}() ) so RecoverProcess runs inside a deferred closure that can call recover, or
alternatively change the RecoverProcess API to return a closure (e.g., func
RecoverProcess(...) func()) and defer the returned function; update any callers
accordingly.
Comment on lines
+199
to
+209
| // IsActive はセッションが「未失効 かつ 未期限切れ」なら true を返す。 | ||
| // 呼び出し側は Status() == "active" を直接見ず、必ずこのメソッドで判定する | ||
| // (status が active のまま expiresAt を過ぎているケースを取りこぼさないため)。 | ||
| func (e *Session) IsActive() bool { | ||
| return e.status.Value() == statusActive.Value() && time.Now().Before(e.expiresAt) | ||
| } | ||
|
|
||
| // IsRevoked はセッションが revoked 状態なら true を返す。 | ||
| func (e *Session) IsRevoked() bool { | ||
| return e.status.Value() == statusRevoked.Value() | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
time.Now() のドメインエンティティ内での直接呼び出しについて
IsActive() 内で time.Now() を直接呼び出しているため、メソッドが非決定的になり、テストでの時刻制御が難しくなります。将来的にテスト容易性を高めたい場合は、時刻を引数として受け取るか、クロックインターフェースを注入する設計を検討してください。
また、e.status.Value() == statusActive.Value() の比較は、e.status == statusActive と直接比較可能であれば簡潔になります。
♻️ 直接比較への簡略化案
func (e *Session) IsActive() bool {
- return e.status.Value() == statusActive.Value() && time.Now().Before(e.expiresAt)
+ return e.status == statusActive && time.Now().Before(e.expiresAt)
}
func (e *Session) IsRevoked() bool {
- return e.status.Value() == statusRevoked.Value()
+ return e.status == statusRevoked
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/internal/domain/identity/entity.go` around lines 199 - 209, The
IsActive method uses time.Now() directly and compares status via Value(), which
makes testing/time control hard and is verbose; change Session.IsActive to
accept a time parameter (e.g., now time.Time) or use an injected clock interface
and use that instead of time.Now(), and simplify the status check from
e.status.Value() == statusActive.Value() to a direct comparison e.status ==
statusActive; ensure IsRevoked uses the same direct-status comparison pattern
(e.status == statusRevoked) for consistency.
Comment on lines
+17
to
+18
| AttrTraceID = "trace_id" | ||
| AttrSpanID = "span_id" |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
trace_id / span_id も sem-conv 範囲外であることを明記
コメントで request.id のみカスタム拡張と記載されていますが、trace_id と span_id もアンダースコア形式で OTel Semantic Conventions の標準 (trace.id 等) とは異なります。ドキュメントの正確性のため、これらもカスタムであることを明記すると良いでしょう。
📝 コメント修正案
-// `request.id` のみ sem-conv 範囲外の独自拡張である。
+// `request.id`, `trace_id`, `span_id` は sem-conv 範囲外の独自拡張である。
+// (sem-conv では trace.id 等のドット区切りだが、ログの可読性のためアンダースコアを採用)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/pkg/obs/attr.go` around lines 17 - 18, Update the comment above the
attribute constants to explicitly state that AttrTraceID and AttrSpanID (in
addition to request.id) are custom attributes that deviate from OpenTelemetry
semantic conventions (which use dot-form keys like "trace.id" and "span.id");
locate the constants AttrTraceID and AttrSpanID and amend the surrounding
comment to mention these are non-standard/extension keys and differ from the
OTel sem-conv names for clarity.
Comment on lines
523
to
+533
| if (this.editorMode === 'edit') { | ||
| await updateArticle(this.form.externalId, this.form) | ||
| await updateArticle(draft.externalId, draft) | ||
| this.successMessage = '記事を更新しました。' | ||
| } else { | ||
| await createArticle(this.form) | ||
| await createArticle(draft) | ||
| this.editorMode = 'edit' | ||
| this.successMessage = '記事を登録しました。' | ||
| } | ||
|
|
||
| this.setBaseline(cloneArticleDraft(this.form)) | ||
| this.setBaseline(cloneArticleDraft(draft)) | ||
| await Promise.all([this.reloadArticles(), this.refreshTags()]) | ||
| this.localDirty = false |
Contributor
There was a problem hiding this comment.
保存成功後に repository 側の dirty 状態が残っています。
handleInput() で this.articleRepo.adminDirty を true にしていますが、成功時は localDirty しか戻していません。これだと保存後も離脱確認が出続け、送信ボタンの活性条件も崩れます。
💡 修正案
this.setBaseline(cloneArticleDraft(draft))
await Promise.all([this.reloadArticles(), this.refreshTags()])
this.localDirty = false
+ this.articleRepo.setAdminDirty(false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/pages/page-admin-articles.ts` around lines 523 - 533, The save
flow leaves the repository-level dirty flag set: handleInput() marks
this.articleRepo.adminDirty = true but the success branch only clears
this.localDirty; update the save logic (the block that calls
updateArticle/createArticle, setBaseline, Promise.all, etc.) to also reset
this.articleRepo.adminDirty = false on successful save (after setBaseline or
after Promise.all) so the repository dirty state, the leave-confirmation and
submit-button enablement are restored; ensure both the 'edit' and create
branches clear articleRepo.adminDirty and keep existing calls to
setBaseline(cloneArticleDraft(draft)), this.localDirty = false, and the
reloadArticles()/refreshTags() Promise.all.
Comment on lines
+560
to
+566
| private handleReset = (e: Event) => { | ||
| e.preventDefault() | ||
| if (this.confirmDiscardChanges()) { | ||
| this.localDirty = false | ||
| this.articleRepo.setAdminDirty(false) | ||
| this.requestUpdate() | ||
| } |
Contributor
There was a problem hiding this comment.
「入力を戻す」が baseline の値を復元していません。
ここも dirty フラグを消しているだけで、フォーム内の <me-*> コンポーネント値は編集前の baseline に戻っていません。未保存の変更が残ったまま「保存済み」表示になるので、リセット時にフォームを再初期化する処理が必要です。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/pages/page-admin-articles.ts` around lines 560 - 566,
handleReset currently only clears dirty flags (this.localDirty,
this.articleRepo.setAdminDirty) but does not restore the form values to the
saved baseline; update handleReset so that after confirmDiscardChanges()
succeeds you reinitialize the form controls from the baseline (e.g., restore
values from this.baseline into all bound fields / <me-*> components), ensure any
per-field state is reset (validation, selected options, etc.), then clear the
dirty flags and call this.requestUpdate(); if there isn't an existing helper,
add a clear/reset method (e.g., resetFormToBaseline or initFormFromBaseline) and
call it from handleReset.
Comment on lines
+211
to
+220
| private handleReset = (e: Event) => { | ||
| e.preventDefault() | ||
| if ( | ||
| this.isDirty && | ||
| this.profileRepo.adminDirty && | ||
| !window.confirm('未保存の変更を破棄して元に戻しますか？') | ||
| ) { | ||
| return | ||
| } | ||
|
|
||
| this.setForm(cloneMeProfile(this.profile)) | ||
| } | ||
|
|
||
| private setForm(nextForm: MeProfile) { | ||
| this.form = nextForm | ||
| this.updateDirtyState(nextForm) | ||
| } | ||
|
|
||
| private updateDirtyState(nextForm: MeProfile) { | ||
| const nextDirty = !this.profilesEqual(nextForm, this.profile) | ||
| if (nextDirty === this.isDirty) return | ||
|
|
||
| this.isDirty = nextDirty | ||
| this.dispatchEvent( | ||
| new CustomEvent<boolean>('admin-profile-dirty-change', { | ||
| detail: nextDirty, | ||
| bubbles: true, | ||
| composed: true, | ||
| }), | ||
| ) | ||
| } | ||
|
|
||
| private profilesEqual(a: MeProfile, b: MeProfile) { | ||
| return JSON.stringify(a) === JSON.stringify(b) | ||
| } | ||
|
|
||
| private renderEmptyPanel( | ||
| message: string, | ||
| actionLabel: string, | ||
| onClick: () => void, | ||
| ) { | ||
| return html`<article class="empty-panel"> | ||
| <p>${message}</p> | ||
| <button type="button" class="subtle" @click=${onClick}>${actionLabel}</button> | ||
| </article>` | ||
| this.profileRepo.setAdminDirty(false) | ||
| this.requestUpdate() |
Contributor
There was a problem hiding this comment.
「変更を元に戻す」が実際の入力値を戻していません。
ここでは dirty フラグを落としているだけで、各 <me-*> フィールドの表示値は adminProfile の内容に戻っていません。現状だと未保存の変更が画面に残ったまま「保存済み」の見た目になるので、フォームを repo のスナップショットから再初期化する処理が必要です。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/pages/page-admin-profile.ts` around lines 211 - 220, handleReset
currently only clears the dirty flag but does not restore the visible form
values; update the handleReset implementation (the method named handleReset) so
after confirming and calling this.profileRepo.setAdminDirty(false) it also
re-initializes the form controls from the repository's canonical data
(this.profileRepo.adminProfile) — e.g., assign each field/component value from
this.profileRepo.adminProfile (or call a repo method that returns a snapshot) so
each <me-*> input reflects adminProfile, then call this.requestUpdate() to
re-render; ensure you reference the this.profileRepo.adminProfile object and the
handleReset method when making the change.
Comment on lines
+188
to
+194
| private renderTagToggle() { | ||
| return html` | ||
| <button type="button" class="tag-toggle" @click=${() => (this.showAllTags = !this.showAllTags)}> | ||
| ${this.showAllTags ? '— show less' : `+ ${this.tagOptions.length - 12} more`} | ||
| </button> | ||
| ` | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
タグトグルボタンにアクセシビリティ属性を追加することを検討してください。
タグクラウドの展開/折りたたみボタンにaria-expanded属性を追加すると、スクリーンリーダーユーザーにとって状態がより明確になります。
♿ 提案される修正
private renderTagToggle() {
return html`
- <button type="button" class="tag-toggle" `@click`=${() => (this.showAllTags = !this.showAllTags)}>
+ <button
+ type="button"
+ class="tag-toggle"
+ aria-expanded=${this.showAllTags}
+ `@click`=${() => (this.showAllTags = !this.showAllTags)}
+ >
${this.showAllTags ? '— show less' : `+ ${this.tagOptions.length - 12} more`}
</button>
`
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/pages/page-articles.ts` around lines 188 - 194, The tag toggle
button in renderTagToggle should expose its expanded/collapsed state to
assistive tech: update the <button> rendered by renderTagToggle to include
aria-expanded="${this.showAllTags}" (and add aria-controls referencing the id of
the tag list element if one exists) so screen readers can determine current
state; keep the existing class "tag-toggle", the click handler that flips
this.showAllTags, and the current label logic using this.tagOptions.length.
- noParameterAssign をエラー指定
fix: PR #88 対応
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/biome.json`:
- Around line 30-33: The new biomes rule "noNonNullAssertion" in the style
object is set to "error" but the codebase still uses non-null assertion patterns
(e.g., class properties declared like "private _foo!: Type" using the "!"
operator), which will break CI; to fix, change the "noNonNullAssertion" value
from "error" to "warn" in the style object of the biome settings so the rule is
linted but does not fail the build, and leave a TODO comment to flip it back to
"error" once all occurrences of the "private _...!: Type" pattern are
refactored.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aa0cbfd6-3526-47e1-b540-705cbb939c71
📒 Files selected for processing (1)
frontend/biome.json
Comment on lines
+30
to
+33
| "style": { | ||
| "noNonNullAssertion": "error", | ||
| "noParameterAssign": "error" | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# noNonNullAssertion に抵触しうる definite assignment assertion の残存確認
rg -nP --type=ts '\b(private|protected|public)\s+[A-Za-z_$][\w$]*\s*!\s*:'
# ルールが error で有効化されていることを確認
rg -nP '"noNonNullAssertion"\s*:\s*"error"' frontend/biome.jsonRepository: umekikazuya/me
Length of output: 597
noNonNullAssertion: "error" の即時適用は現状コードと不整合です。
Line 31 で error に設定されていますが、frontend/src/pages/page-top.ts、page-admin-profile.ts、page-admin-login.ts、page-admin-articles.ts、page-admin-account.ts、page-about.ts など複数ファイルに private _...!: type パターンの非nullアサーションが残っており、リント失敗によりビルドゲートが落とされるリスクがあります。同PRで既存違反をすべて解消するか、段階移行として一時的に warn に設定してください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/biome.json` around lines 30 - 33, The new biomes rule
"noNonNullAssertion" in the style object is set to "error" but the codebase
still uses non-null assertion patterns (e.g., class properties declared like
"private _foo!: Type" using the "!" operator), which will break CI; to fix,
change the "noNonNullAssertion" value from "error" to "warn" in the style object
of the biome settings so the rule is linted but does not fail the build, and
leave a TODO comment to flip it back to "error" once all occurrences of the
"private _...!: Type" pattern are refactored.
refactor: 仕様書に基づいてコンポーネントを再設計
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (10)
frontend/src/domain/ProfileRepository.ts (3)
145-147:⚠️ Potential issue | 🟠 Major公開用と管理用で同じ
MeProfile参照を共有しています。ここでは同じオブジェクトを
publicProfileとadminProfileに入れているため、管理画面での破壊的編集が保存前でも公開側 state に漏れます。公開用と管理用は必ず別インスタンスで保持してください。Also applies to: 160-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/domain/ProfileRepository.ts` around lines 145 - 147, The current code assigns the same MeProfile object to both _publicProfile and _adminProfile, causing destructive edits in the admin view to leak into the public view; change assignments that use the fetched value (the assignment of p from this._fetchPromise and the similar assignments at the other occurrence) to create separate instances instead of sharing the same reference (e.g., construct a new MeProfile/new object or deep-clone p for one of the targets) so that _publicProfile and _adminProfile each hold independent MeProfile instances.
141-150:⚠️ Potential issue | 🟠 Major進行中の
getMe()が保存成功後の最新 state を後から上書きします。
_internalFetch()の完了前にsaveAdminProfile()が成功すると、遅れて返った古いgetMe()結果が Line 146-147 で両 state を再代入します。保存開始時に進行中 fetch を失効させるか、fetch 世代を比較して古い結果を破棄する必要があります。Also applies to: 154-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/domain/ProfileRepository.ts` around lines 141 - 150, The issue is that an in-flight getMe() awaited in _internalFetch can overwrite newer state set by saveAdminProfile; to fix, add a generation or token check around _fetchPromise results: introduce a numeric _fetchGeneration incremented whenever a saveAdminProfile (or any state-changing write) starts, capture the current generation before awaiting getMe() inside _internalFetch and only assign _publicProfile/_adminProfile if the captured generation still equals the instance _fetchGeneration; alternatively, invalidate the in-flight fetch by bumping/clearing _fetchPromise when saveAdminProfile begins so the old fetch result is discarded—apply the same generation/check pattern to the other affected block (the 154-166 region).
122-129:⚠️ Potential issue | 🟡 Minor公開ロード済みでも管理側が同じプロフィールを再取得します。
_internalFetch()は Line 146-147 でadminProfileも埋めていますが、公開側から取得したケースでは_adminLoadedが立たないため、管理画面の初回表示でgetMe()がもう一度走ります。adminProfileの実データ有無でもガードするか、共有 fetch 成功時に管理側も loaded 扱いにしてください。Also applies to: 141-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/domain/ProfileRepository.ts` around lines 122 - 129, loadAdminProfile is re-triggering admin fetch even when the same profile was already loaded by the public flow because _internalFetch populates adminProfile but doesn’t set _adminLoaded; update the logic so that either loadAdminProfile first returns early when adminProfile already contains real data (e.g., non-empty id/email) or modify _internalFetch’s success path to set this._adminLoaded = true (and clear this._adminLoading/this._adminError) when it populates adminProfile, ensuring getMe() won’t run again; adjust both the loadAdminProfile guard and the shared _internalFetch success handling to keep state consistent between public and admin flows.frontend/src/components/admin/ui/me-select.ts (3)
10-10:⚠️ Potential issue | 🟠 Majorフォームリセットで既定値を失います。
ここも
formResetCallback()が空文字固定のままです。valueを反映属性にしているため、既定値は別に保持しないとネイティブselect相当のリセット動作になりません。修正案
export class MeSelect extends LitElement { static formAssociated = true + private _defaultValue = '' @@ constructor() { super() this._internals = this.attachInternals() } + + connectedCallback() { + super.connectedCallback() + this._defaultValue = this.value ?? '' + } @@ formResetCallback() { - this.value = '' + this.value = this._defaultValue this._syncInternals() }For form-associated custom elements, should formResetCallback restore the control's initial value rather than always setting an empty string? How does reflecting a live `value` property to an attribute affect reset semantics?Also applies to: 26-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/ui/me-select.ts` at line 10, The component currently hardcodes formResetCallback to set value = '' which breaks native-like reset because value is reflected to an attribute; change formResetCallback to restore an initialValue snapshot instead of clearing it: capture the initial value when the element is constructed or in connectedCallback (read the existing this.value or attribute reflected value) and store it on a private field (e.g., this._initialValue), then implement formResetCallback to set this.value = this._initialValue; update any other similar components (the same pattern around the reflected `@property` value and formResetCallback at the other occurrences) to use the same initial-value restore logic rather than assigning an empty string.
11-12:⚠️ Potential issue | 🟠 Major
required/disabled変更時に妥当性が再同期されません。今も
value変更時しか_syncInternals()を呼ばないので、必須状態の切り替え後にcheckValidity()が古いまま残ります。前回指摘の未解消です。修正案
protected updated(changedProperties: Map<PropertyKey, unknown>) { - if (changedProperties.has('value')) { + if ( + changedProperties.has('value') || + changedProperties.has('required') || + changedProperties.has('disabled') + ) { this._syncInternals() } }For form-associated custom elements using ElementInternals.setValidity(), do `required` and `disabled` property changes need explicit re-synchronization, or is syncing only on value changes sufficient?Also applies to: 49-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/ui/me-select.ts` around lines 11 - 12, The component fails to resync form validity when the Boolean properties required or disabled change because _syncInternals() is only invoked on value changes; update the element to call _syncInternals() whenever required or disabled changes (e.g., in the property setters or in updated(changedProps) by checking changedProps.has('required') || changedProps.has('disabled')), so that ElementInternals.setValidity() and checkValidity() are refreshed after toggling required/disabled (ensure the same logic used for value changes is applied; relevant symbols: required, disabled, _syncInternals(), checkValidity(), ElementInternals.setValidity()).
69-88:⚠️ Potential issue | 🟠 Majorラベル未指定時のアクセシブルネームがありません。
labelが空だと、このselectは読み上げ名を持ちません。前回指摘どおり、aria-labelかaria-labelledbyのフォールバックが必要です。For a custom element rendering a shadow DOM <select>, is an accessible-name fallback required when no visible <label> is rendered?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/ui/me-select.ts` around lines 69 - 88, The select lacks an accessible name when this.label is empty; in render() update the <select> so that when this.label is falsy it receives a fallback accessible name (either via aria-labelledby pointing to the rendered <label> id when present or an aria-label using an existing property such as this.name or a dedicated this.ariaLabel). Concretely, inside render() where the <select> is created (and you reference this._inputId, .name, .value, etc.), add conditional attributes: if this.label render the <label id=${this._inputId + '-label'}> and set aria-labelledby on <select>, otherwise set aria-label on <select> to this.ariaLabel || this.name (or another explicit fallback) so the control always has an accessible name.frontend/src/components/admin/ui/me-textarea.ts (3)
14-14:⚠️ Potential issue | 🟠 Majorフォームリセットで初期値を復元してください。
formResetCallback()が今も空文字固定です。valueが反映属性なので、現状のままだとリセット先の既定値も保持できません。前回指摘のまま未解消です。修正案
export class MeTextarea extends LitElement { static formAssociated = true + private _defaultValue = '' @@ constructor() { super() this._internals = this.attachInternals() } + + connectedCallback() { + super.connectedCallback() + this._defaultValue = this.value ?? '' + } @@ formResetCallback() { - this.value = '' + this.value = this._defaultValue this._syncInternals() }For form-associated custom elements, should formResetCallback restore the control's initial value rather than always setting an empty string? How does reflecting a live `value` property to an attribute affect reset semantics?Also applies to: 31-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/ui/me-textarea.ts` at line 14, formResetCallback currently always sets an empty string and ignores the reflected value; capture the element's initial value (e.g., read this.value or this.getAttribute('value') into a private field like _initialValue during construction or connectedCallback) and change formResetCallback to restore that stored _initialValue by assigning it back to this.value so the reflected attribute and reset semantics are preserved (apply the same change for the other occurrences around lines 31-34).
54-58:⚠️ Potential issue | 🟠 Major妥当性再同期が
value変更時だけのままです。
required/disabledの変更後もElementInternalsが再同期されないので、checkValidity()と UI が次の入力までズレます。これも前回指摘の未解消です。修正案
protected updated(changedProperties: Map<PropertyKey, unknown>) { - if (changedProperties.has('value')) { + if ( + changedProperties.has('value') || + changedProperties.has('required') || + changedProperties.has('disabled') + ) { this._syncInternals() } }For form-associated custom elements using ElementInternals.setValidity(), do `required` and `disabled` property changes need explicit re-synchronization, or is syncing only on value changes sufficient?Also applies to: 60-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/ui/me-textarea.ts` around lines 54 - 58, The updated() override only re-syncs ElementInternals when 'value' changes; update it to also detect changes to 'required' and 'disabled' so the internals and validity UI stay in sync: in the updated(changedProperties) method of this component call this._syncInternals() when changedProperties.has('value') || changedProperties.has('required') || changedProperties.has('disabled') (and ensure any attribute/property mirror for required/disabled triggers property changes so this check catches both).
40-52:⚠️ Potential issue | 🟠 Major
changeを毎入力で発火する実装のままです。
textareaの確定前入力で親コンポーネントが反応してしまいます。前回指摘どおり、changeは commit 時だけ再送してください。修正案
private _onInput(e: Event) { const input = e.target as HTMLTextAreaElement this.value = input.value this._syncInternals() - - this.dispatchEvent( - new CustomEvent('change', { - detail: input.value, - bubbles: true, - composed: true, - }), - ) } + + private _onChange(e: Event) { + e.stopPropagation() + this.dispatchEvent( + new CustomEvent('change', { + detail: this.value, + bubbles: true, + composed: true, + }), + ) + } @@ .placeholder=${this.placeholder} ?disabled=${this.disabled} ?required=${this.required} `@input`=${this._onInput} + `@change`=${this._onChange} ></textarea>In a custom element wrapping a <textarea>, should a `change` event be re-dispatched on every `input` event, or only when the native textarea fires `change` on commit/blur?Also applies to: 82-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/ui/me-textarea.ts` around lines 40 - 52, The component currently re-dispatches a 'change' CustomEvent inside _onInput on every keystroke; stop re-emitting 'change' on input and only re-dispatch when the native textarea commits (i.e., in the native 'change' handler or on blur). Update _onInput to only update this.value and call _syncInternals (remove the dispatch block), and move the CustomEvent dispatch into the existing native-change handler (or implement an _onChange/_onBlur that listens to the textarea's 'change' and dispatches new CustomEvent('change', { detail: this.value, bubbles: true, composed: true })). Apply the same change for the second occurrence noted (lines ~82-92) so only commit events re-broadcast as 'change'.frontend/src/components/admin/ui/me-text-input.ts (1)
165-168:⚠️ Potential issue | 🟡 Minor
:placeholder-shown依存の無効スタイルはまだ脆いです。
placeholderが無い利用箇所では初期表示から無効スタイルになる可能性があります。前回指摘と同じ根本原因が残っています。Is using `:invalid:not(:placeholder-shown)` safe for validation styling when a placeholder attribute may be absent?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/admin/ui/me-text-input.ts` around lines 165 - 168, The invalid-style selector relies on :placeholder-shown and will incorrectly show error state on inputs that have no placeholder; update the selector and add a fallback: change the rule to target only inputs that actually have a placeholder (e.g. input[placeholder]:invalid:not(:placeholder-shown)) and also add a fallback rule for placeholder-less fields such as input:invalid:focus (or input:invalid:not(:focus)) so inputs without a placeholder don't show error styles on initial render; update the selector(s) where input:invalid:not(:placeholder-shown) is used accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/admin/ui/me-text-input.ts`:
- Around line 17-28: The ElementInternals validity is only updated on value
changes; update the component so changes to required, disabled, and type also
call _syncInternals() (and thus setValidity())—for example, detect changes to
the properties declared (required, disabled, type) in the component’s
update/updated callback or add property observers and invoke _syncInternals()
when any of these properties change; reference the me-text-input class, its
_syncInternals() method, and the required/disabled/type properties to locate
where to add these calls.
- Around line 53-64: 現在の _onInput() は毎キーで CustomEvent('change') を発火しており、ネイティブの
change 意味論を壊しています; 代わりに _onInput() はこの.value を更新して _syncInternals()
を呼ぶだけにし、もし外部にリアルタイム通知が必要なら CustomEvent('input') を送るか別途用意すること（例: dispatch
CustomEvent('input') with detail input.value）；CustomEvent('change') の発火は
blur/確定タイミングのハンドラ（例: _onBlur() や commitValue()）に移動して、その場で detail に最終値を含めて
dispatch してください。
- Line 16: The component's formResetCallback always sets this.value to '' and
thus discards any initial bound value; create a separate field (e.g.
defaultValue) to hold the initial value and initialize it from the reflected
attribute/property when the element is first connected or when the attribute is
first set, ensure user edits update only this.value (not defaultValue), and
change formResetCallback to restore this.value = this.defaultValue so form reset
returns to the original initial value; refer to the existing property "value",
the method "formResetCallback", and the component class (me-text-input) when
implementing this.
In `@frontend/src/components/admin/ui/me-textarea.ts`:
- Around line 140-142: The CSS selector textarea:invalid:not(:placeholder-shown)
causes false-positive error styling when the placeholder is absent or empty;
update the code in me-textarea (and the similar me-text-input) to either ensure
required inputs have a non-empty placeholder (e.g. placeholder=" ") so
:placeholder-shown behaves correctly, or replace the selector with the newer
:user-invalid pseudo-class (e.g. textarea:user-invalid) to target user-caused
validation only; change the selector and ensure both me-textarea and
me-text-input use the same approach for consistency.
In `@frontend/src/components/app-root.ts`:
- Around line 166-176: The route handler handleRouteChange currently only
refreshes the session when auth.status === 'unknown' and therefore misses the
case where auth.status === 'guest' trying to access a protected admin path;
update handleRouteChange to also detect when this.currentPath is a protected
admin path (use isProtectedAdminPath(this.currentPath)) and auth.status ===
'guest' and in that case navigate/redirect to '/admin/login' (or otherwise
trigger the same redirect flow as the /admin/login branch) so requests to
protected routes like /admin/profile do not end up with me-auth-guard returning
nothing.
In `@frontend/src/controllers/RepositoryObserver.ts`:
- Around line 22-23: The RepositoryObserver is left registered as a Lit
controller (host.addController(this)) and disconnect() only removes event
listeners, so old observers remain in the host's controller list and continue to
receive hostConnected(); fix by either unregistering the controller in your
teardown or switching to reusing the observer: add a host.removeController(this)
call when you intend to dispose the observer (e.g., inside
RepositoryObserver.disconnect or in the same place you remove listeners), or
refactor RepositoryObserver to expose a method like setRepository(newRepo) and
update callers (e.g., places creating new observers such as me-auth-guard.ts) to
reuse a single observer and call setRepository instead of
instantiating/registering a new controller.
In `@frontend/src/domain/AuthRepository.ts`:
- Around line 99-115: Concurrent refreshSession() results can overwrite a newer
auth state set by login(); to fix, add a generation/version or token check to
ignore stale refresh results: increment a private authGeneration (or snapshot
current token) at the start of any top-level auth operation (e.g. login()) and
have refreshSession() capture that generation/token before awaiting; when the
refresh result resolves, compare the captured generation/token to the current
one and only apply changes to _status/_loginError/_loginNotice if they match,
otherwise discard the stale result. Ensure the same guard is used where
refreshSession() updates state so any in-flight refresh is invalidated when
another auth op begins.
- Around line 175-187: In changeEmail(), clear previous result state at the
start by setting this._accountError = '' and this._accountSuccess = '' (then
call notifyChange) before setting this._accountBusyAction; this ensures old
success/failure messages don't persist into a new attempt while keeping the
existing try/catch/finally logic (refer to changeEmail, _accountError,
_accountSuccess, _accountBusyAction, notifyChange, apiChangeEmail).
---
Duplicate comments:
In `@frontend/src/components/admin/ui/me-select.ts`:
- Line 10: The component currently hardcodes formResetCallback to set value = ''
which breaks native-like reset because value is reflected to an attribute;
change formResetCallback to restore an initialValue snapshot instead of clearing
it: capture the initial value when the element is constructed or in
connectedCallback (read the existing this.value or attribute reflected value)
and store it on a private field (e.g., this._initialValue), then implement
formResetCallback to set this.value = this._initialValue; update any other
similar components (the same pattern around the reflected `@property` value and
formResetCallback at the other occurrences) to use the same initial-value
restore logic rather than assigning an empty string.
- Around line 11-12: The component fails to resync form validity when the
Boolean properties required or disabled change because _syncInternals() is only
invoked on value changes; update the element to call _syncInternals() whenever
required or disabled changes (e.g., in the property setters or in
updated(changedProps) by checking changedProps.has('required') ||
changedProps.has('disabled')), so that ElementInternals.setValidity() and
checkValidity() are refreshed after toggling required/disabled (ensure the same
logic used for value changes is applied; relevant symbols: required, disabled,
_syncInternals(), checkValidity(), ElementInternals.setValidity()).
- Around line 69-88: The select lacks an accessible name when this.label is
empty; in render() update the <select> so that when this.label is falsy it
receives a fallback accessible name (either via aria-labelledby pointing to the
rendered <label> id when present or an aria-label using an existing property
such as this.name or a dedicated this.ariaLabel). Concretely, inside render()
where the <select> is created (and you reference this._inputId, .name, .value,
etc.), add conditional attributes: if this.label render the <label
id=${this._inputId + '-label'}> and set aria-labelledby on <select>, otherwise
set aria-label on <select> to this.ariaLabel || this.name (or another explicit
fallback) so the control always has an accessible name.
In `@frontend/src/components/admin/ui/me-text-input.ts`:
- Around line 165-168: The invalid-style selector relies on :placeholder-shown
and will incorrectly show error state on inputs that have no placeholder; update
the selector and add a fallback: change the rule to target only inputs that
actually have a placeholder (e.g.
input[placeholder]:invalid:not(:placeholder-shown)) and also add a fallback rule
for placeholder-less fields such as input:invalid:focus (or
input:invalid:not(:focus)) so inputs without a placeholder don't show error
styles on initial render; update the selector(s) where
input:invalid:not(:placeholder-shown) is used accordingly.
In `@frontend/src/components/admin/ui/me-textarea.ts`:
- Line 14: formResetCallback currently always sets an empty string and ignores
the reflected value; capture the element's initial value (e.g., read this.value
or this.getAttribute('value') into a private field like _initialValue during
construction or connectedCallback) and change formResetCallback to restore that
stored _initialValue by assigning it back to this.value so the reflected
attribute and reset semantics are preserved (apply the same change for the other
occurrences around lines 31-34).
- Around line 54-58: The updated() override only re-syncs ElementInternals when
'value' changes; update it to also detect changes to 'required' and 'disabled'
so the internals and validity UI stay in sync: in the updated(changedProperties)
method of this component call this._syncInternals() when
changedProperties.has('value') || changedProperties.has('required') ||
changedProperties.has('disabled') (and ensure any attribute/property mirror for
required/disabled triggers property changes so this check catches both).
- Around line 40-52: The component currently re-dispatches a 'change'
CustomEvent inside _onInput on every keystroke; stop re-emitting 'change' on
input and only re-dispatch when the native textarea commits (i.e., in the native
'change' handler or on blur). Update _onInput to only update this.value and call
_syncInternals (remove the dispatch block), and move the CustomEvent dispatch
into the existing native-change handler (or implement an _onChange/_onBlur that
listens to the textarea's 'change' and dispatches new CustomEvent('change', {
detail: this.value, bubbles: true, composed: true })). Apply the same change for
the second occurrence noted (lines ~82-92) so only commit events re-broadcast as
'change'.
In `@frontend/src/domain/ProfileRepository.ts`:
- Around line 145-147: The current code assigns the same MeProfile object to
both _publicProfile and _adminProfile, causing destructive edits in the admin
view to leak into the public view; change assignments that use the fetched value
(the assignment of p from this._fetchPromise and the similar assignments at the
other occurrence) to create separate instances instead of sharing the same
reference (e.g., construct a new MeProfile/new object or deep-clone p for one of
the targets) so that _publicProfile and _adminProfile each hold independent
MeProfile instances.
- Around line 141-150: The issue is that an in-flight getMe() awaited in
_internalFetch can overwrite newer state set by saveAdminProfile; to fix, add a
generation or token check around _fetchPromise results: introduce a numeric
_fetchGeneration incremented whenever a saveAdminProfile (or any state-changing
write) starts, capture the current generation before awaiting getMe() inside
_internalFetch and only assign _publicProfile/_adminProfile if the captured
generation still equals the instance _fetchGeneration; alternatively, invalidate
the in-flight fetch by bumping/clearing _fetchPromise when saveAdminProfile
begins so the old fetch result is discarded—apply the same generation/check
pattern to the other affected block (the 154-166 region).
- Around line 122-129: loadAdminProfile is re-triggering admin fetch even when
the same profile was already loaded by the public flow because _internalFetch
populates adminProfile but doesn’t set _adminLoaded; update the logic so that
either loadAdminProfile first returns early when adminProfile already contains
real data (e.g., non-empty id/email) or modify _internalFetch’s success path to
set this._adminLoaded = true (and clear this._adminLoading/this._adminError)
when it populates adminProfile, ensuring getMe() won’t run again; adjust both
the loadAdminProfile guard and the shared _internalFetch success handling to
keep state consistent between public and admin flows.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d8b42d10-f1ff-4ac0-ad61-076801a552a8
📒 Files selected for processing (11)
frontend/index.htmlfrontend/src/components/admin/ui/me-auth-guard.tsfrontend/src/components/admin/ui/me-select.tsfrontend/src/components/admin/ui/me-text-input.tsfrontend/src/components/admin/ui/me-textarea.tsfrontend/src/components/app-admin-shell.tsfrontend/src/components/app-root.tsfrontend/src/controllers/RepositoryObserver.tsfrontend/src/domain/ArticleRepository.tsfrontend/src/domain/AuthRepository.tsfrontend/src/domain/ProfileRepository.ts
| @property({ reflect: true }) label = '' | ||
| @property({ reflect: true }) name = '' | ||
| @property({ reflect: true }) autocomplete = '' | ||
| @property({ reflect: true }) value: string | number = '' |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For form-associated custom elements, should formResetCallback restore the control's initial value rather than always setting an empty string? How does reflecting a live value property to an attribute affect reset semantics?
💡 Result:
For form-associated custom elements (FACE), the formResetCallback should restore the control's initial/default state as defined by the element author, rather than always setting an empty string. The HTML specification defines the reset algorithm for FACE as enqueuing a custom element callback reaction named "formResetCallback" with an empty argument list. This callback has no predefined behavior; authors implement it to match native form control reset semantics, typically restoring to the value from the value attribute (if present) or an empty/default state. Native resettable elements (input, select, textarea, output) reset to their initial attribute values: e.g., input sets value to the value attribute's value or empty string. FACE authors should mimic this in formResetCallback, often by checking this.getAttribute('value') ?? '' and using internals.setFormValue. Reflecting a live value property to the value attribute does not directly affect reset semantics. Reflection is author-defined via observedAttributes and attributeChangedCallback, typically updating internal state or UI from the attribute. On form reset, formResetCallback runs independently; if it reads the attribute (common practice), reflection ensures consistency between property changes and attribute state. However, the spec does not require or assume reflection—reset behavior depends solely on the callback implementation. Native elements reflect value <-> attribute bidirectionally, but FACE value is optional and separate from form value (set via internals.setFormValue). Examples from practice: - Restore from attribute: formResetCallback { this.#setValue(this.getAttribute('value') ?? ''); } - Capture initial state in firstUpdated/connectedCallback and restore it. - For selects/sliders: Reset to defaultSelected or captured defaults. This matches native behavior where reset restores HTML-declared initial state, not always empty.
Citations:
- 1: https://html.spec.whatwg.org/custom-elements.html
- 2: https://html.spec.whatwg.org/C/forms.html
- 3: Should light DOM form controls in a form-associated custom element be ignored during form reset? whatwg/html#10290
- 4: Reset algorithm doesn't work in form-associated custom elements whatwg/html#10927
- 5: https://frontendmasters.com/blog/form-associated-custom-elements-in-practice/
- 6: https://www.whatwg.org/specs/web-apps/current-work/complete/input.html
🏁 Script executed:
find . -type f -name "me-text-input.ts" | head -5Repository: umekikazuya/me
Length of output: 110
🏁 Script executed:
cat -n ./frontend/src/components/admin/ui/me-text-input.tsRepository: umekikazuya/me
Length of output: 5664
🏁 Script executed:
# Check if there are any tests for me-text-input
find . -type f -name "*me-text-input*test*" -o -name "*test*me-text-input*" 2>/dev/null
# Also check for any usage of me-text-input in the codebase
rg "me-text-input" --type ts --type html -l | head -20Repository: umekikazuya/me
Length of output: 547
🏁 Script executed:
# Check one usage example to understand how initial values might be set
rg -A 5 "me-text-input" frontend/src/pages/page-admin-login.ts | head -30Repository: umekikazuya/me
Length of output: 958
🏁 Script executed:
# Check usage in profile page which likely has initial values
rg -B 5 -A 10 "me-text-input" frontend/src/pages/page-admin-profile.ts | head -50Repository: umekikazuya/me
Length of output: 1972
🏁 Script executed:
# Check if there's any test for reset behavior
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) | xargs grep -l "me-text-input\|formReset" 2>/dev/null | head -5Repository: umekikazuya/me
Length of output: 40
フォームリセットで初期値を失わないようにしてください。
formResetCallback() が常に '' に戻すため、初期値つきフォームでネイティブ input と異なる挙動になります。page-admin-profile.ts などでプロパティバインディング経由で初期値を設定している場合、フォームリセット時に初期値が失われます。value を反映属性にしているため、ユーザーが編集した後は属性から初期値を復元できません。HTML フォーム関連カスタム要素の仕様に従い、初期値は別フィールドで保持してください。
修正案
export class MeTextInput extends LitElement {
static formAssociated = true
+ private _defaultValue = ''
`@property`({ reflect: true }) label = ''
`@property`({ reflect: true }) name = ''
@@
constructor() {
super()
this._internals = this.attachInternals()
}
+
+ connectedCallback() {
+ super.connectedCallback()
+ this._defaultValue = String(this.value ?? '')
+ }
@@
formResetCallback() {
- this.value = ''
+ this.value = this._defaultValue
this._syncInternals()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/admin/ui/me-text-input.ts` at line 16, The
component's formResetCallback always sets this.value to '' and thus discards any
initial bound value; create a separate field (e.g. defaultValue) to hold the
initial value and initialize it from the reflected attribute/property when the
element is first connected or when the attribute is first set, ensure user edits
update only this.value (not defaultValue), and change formResetCallback to
restore this.value = this.defaultValue so form reset returns to the original
initial value; refer to the existing property "value", the method
"formResetCallback", and the component class (me-text-input) when implementing
this.
Comment on lines
+17
to
+28
| @property({ reflect: true }) type: | ||
| | 'text' | ||
| | 'number' | ||
| | 'email' | ||
| | 'password' | ||
| | 'url' | ||
| | 'datetime-local' | ||
| | 'search' = 'text' | ||
| @property({ type: Boolean, reflect: true }) disabled = false | ||
| @property({ type: Boolean, reflect: true }) required = false | ||
| @property({ type: Boolean, reflect: true }) readonly = false | ||
| @property({ reflect: true }) placeholder = '' |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For form-associated custom elements using ElementInternals.setValidity(), do required, disabled, and type property changes need explicit re-synchronization, or is syncing only on value changes sufficient?
💡 Result:
For form-associated custom elements using ElementInternals.setValidity, changes to the required, disabled, and type properties/attributes require explicit re-synchronization via calling setValidity or related methods. Syncing only on value changes (e.g., via setFormValue) is not sufficient. The browser does not automatically reflect HTML attributes like required, disabled, or type into ElementInternals for validation purposes. Custom elements must manually manage validation state. - required: Manually check this.internals.willValidate && this.required && !this.value, then setValidity({valueMissing: true}). Examples show explicit handling in property setters or update methods. - disabled: Managed via formDisabledCallback, but validation state (willValidate) may need manual sync if changing the disabled attribute directly. - type: No automatic effect; custom elements emulate type-specific validation manually. All examples from MDN, WebKit, Frontend Masters, and libraries (Stencil, Lit mixins) call setValidity explicitly on value changes, required checks, or property updates, not relying on automatic sync. HTML spec recognizes these attributes on form-associated custom elements but requires authors to implement behavior via ElementInternals APIs. ARIA equivalents (ariaRequired, ariaDisabled) exist and reflect, but standard form attrs like required do not auto-affect validity. Sources confirm developers must re-call setValidity after attribute/property changes to update :valid/:invalid states and constraint validation.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/ElementInternals
- 2: https://webkit.org/blog/13711/elementinternals-and-form-associated-custom-elements/
- 3: https://frontendmasters.com/blog/form-associated-custom-elements-in-practice/
- 4: https://developer.mozilla.org/en-US/docs/Web/API/ElementInternals/validity
- 5: https://stenciljs.com/docs/form-associated
- 6: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/input
- 7: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/attachInternals
- 8: https://thedevexchange.com/element-internals-api/
- 9: https://html.spec.whatwg.org/custom-elements.html
- 10: https://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html
🏁 Script executed:
# First, find the file and check its existence
find . -name "me-text-input.ts" -type fRepository: umekikazuya/me
Length of output: 110
🏁 Script executed:
# Read the file to see the current implementation
cat -n ./frontend/src/components/admin/ui/me-text-input.tsRepository: umekikazuya/me
Length of output: 5664
required、disabled、type プロパティの変更時にも _syncInternals() を呼び出してください。
現在の実装では value 変更時のみ setValidity() が更新されていますが、required、disabled、type の切り替え時に内部の <input> 要素の validity は変わります。ElementInternals への反映がないため、次の入力まで form.checkValidity() と表示状態が古いままになります。
修正案
protected updated(changedProperties: Map<PropertyKey, unknown>) {
- if (changedProperties.has('value')) {
+ if (
+ changedProperties.has('value') ||
+ changedProperties.has('required') ||
+ changedProperties.has('disabled') ||
+ changedProperties.has('type')
+ ) {
this._syncInternals()
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/admin/ui/me-text-input.ts` around lines 17 - 28, The
ElementInternals validity is only updated on value changes; update the component
so changes to required, disabled, and type also call _syncInternals() (and thus
setValidity())—for example, detect changes to the properties declared (required,
disabled, type) in the component’s update/updated callback or add property
observers and invoke _syncInternals() when any of these properties change;
reference the me-text-input class, its _syncInternals() method, and the
required/disabled/type properties to locate where to add these calls.
Comment on lines
+53
to
+64
| private _onInput(e: Event) { | ||
| const input = e.target as HTMLInputElement | ||
| this.value = input.value | ||
| this._syncInternals() | ||
|
|
||
| this.dispatchEvent( | ||
| new CustomEvent('change', { | ||
| detail: input.value, | ||
| bubbles: true, | ||
| composed: true, | ||
| }), | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In a custom element wrapping an <input>, should a changeevent be re-dispatched on everyinputevent, or only when the native input fireschange on commit/blur?
💡 Result:
Only when the native input fires change on commit/blur. The native change event fires specifically when the value is committed (e.g., on blur for text inputs after user edit, or selection changes), not on every keystroke. The input event fires continuously during typing. In a custom element wrapper (often using Shadow DOM), to match native semantics and avoid excessive dispatches or performance issues from frequent events, re-dispatch or proxy the change event only when the native input fires it—not on every input event. This preserves expected behavior: consumers listening for change on your custom element get notified only on value commits, just like a native input. Frequent input events can be proxied separately if real-time updates are needed, but change should align with native timing to prevent surprises (e.g., forms validating prematurely or unnecessary re-renders). Native change does not cross Shadow DOM boundaries (composed: false), so manual re-dispatch from the host is required with bubbles: true, composed: true. For form-associated custom elements (FACE), use ElementInternals.setFormValue on input events to keep form state updated continuously (for submission/validity), but still proxy change only on native change for event consistency.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/DOM/element.onchange
- 2: https://developer.mozilla.org/en-US/docs/DOM/DOM_event_reference/input
- 3: https://lit.dev/docs/v3/components/events
- 4: Change event not bubbling outside Shadow DOM lit/lit-element#922
- 5: https://jschof.dev/posts/2024/4/web-components-and-you-8
- 6: https://stackoverflow.com/questions/54891122/creating-a-custom-input-field-with-web-components-without-outside-accessability
- 7: [Bug]: cds-text-input: native change event does not cross shadow DOM boundary carbon-design-system/carbon#21577
🏁 Script executed:
# First, find and read the specific file
cat -n frontend/src/components/admin/ui/me-text-input.ts | sed -n '40,120p'Repository: umekikazuya/me
Length of output: 2579
change イベントを入力ごとに発火しないでください。
_onInput() で毎キー入力ごとに CustomEvent('change') を送ると、標準の change 意味論が崩れて親側が未確定入力で反応します。ネイティブの change イベント（blur/値の確定時）のタイミングでのみ再送するべきです。
修正案
private _onInput(e: Event) {
const input = e.target as HTMLInputElement
this.value = input.value
this._syncInternals()
-
- this.dispatchEvent(
- new CustomEvent('change', {
- detail: input.value,
- bubbles: true,
- composed: true,
- }),
- )
}
+
+ private _onChange(e: Event) {
+ e.stopPropagation()
+ this.dispatchEvent(
+ new CustomEvent('change', {
+ detail: this.value,
+ bubbles: true,
+ composed: true,
+ }),
+ )
+ }
@@
?disabled=${this.disabled}
?required=${this.required}
?readonly=${this.readonly}
`@input`=${this._onInput}
+ `@change`=${this._onChange}
/>📝 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
| private _onInput(e: Event) { | |
| const input = e.target as HTMLInputElement | |
| this.value = input.value | |
| this._syncInternals() | |
| this.dispatchEvent( | |
| new CustomEvent('change', { | |
| detail: input.value, | |
| bubbles: true, | |
| composed: true, | |
| }), | |
| ) | |
| private _onInput(e: Event) { | |
| const input = e.target as HTMLInputElement | |
| this.value = input.value | |
| this._syncInternals() | |
| } | |
| private _onChange(e: Event) { | |
| e.stopPropagation() | |
| this.dispatchEvent( | |
| new CustomEvent('change', { | |
| detail: this.value, | |
| bubbles: true, | |
| composed: true, | |
| }), | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/admin/ui/me-text-input.ts` around lines 53 - 64, 現在の
_onInput() は毎キーで CustomEvent('change') を発火しており、ネイティブの change 意味論を壊しています; 代わりに
_onInput() はこの.value を更新して _syncInternals() を呼ぶだけにし、もし外部にリアルタイム通知が必要なら
CustomEvent('input') を送るか別途用意すること（例: dispatch CustomEvent('input') with detail
input.value）；CustomEvent('change') の発火は blur/確定タイミングのハンドラ（例: _onBlur() や
commitValue()）に移動して、その場で detail に最終値を含めて dispatch してください。
Comment on lines
+140
to
+142
| textarea:invalid:not(:placeholder-shown) { | ||
| border-color: var(--color-danger); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is using :invalid:not(:placeholder-shown) safe for validation styling when a placeholder attribute may be absent?
💡 Result:
No, using :invalid:not(:placeholder-shown) is not safe for validation styling when the placeholder attribute may be absent. The :placeholder-shown pseudo-class matches or <textarea> elements that are currently displaying placeholder text, which requires both a non-empty placeholder attribute and an empty value. If the placeholder attribute is absent or empty, the element never matches :placeholder-shown, even when empty. Thus, :not(:placeholder-shown) always matches such elements, regardless of whether they have a value. For an empty required input without placeholder: - It matches :invalid (due to required). - It matches :not(:placeholder-shown) (since no placeholder). - So it matches :invalid:not(:placeholder-shown), applying error styling to empty fields without placeholders, which defeats the purpose of using this selector to avoid styling untouched empty fields. Authoritative sources confirm this behavior and recommend adding placeholder=" " (space) to all relevant inputs for the selector to work as intended. Modern alternative: use :user-invalid (widely supported by 2026), which only styles after user interaction. Example from MDN demo: input without placeholder (first field) does not match :placeholder-shown, while those with do when empty.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/CSS/:placeholder-shown
- 2: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Selectors/:placeholder-shown
- 3: https://www.bram.us/2021/01/28/form-validation-you-want-notfocusinvalid-not-invalid/
- 4: https://stackoverflow.com/questions/56758566/is-there-a-css-selector-for-input-field-that-contains-input
- 5: https://stackoverflow.com/questions/55427966/inputnotplaceholder-shown-label-selector-does-not-work-with-autofill
🏁 Script executed:
# Find and read the me-textarea.ts file to check the actual implementation around lines 140-142
fd -t f "me-textarea.ts" | head -5Repository: umekikazuya/me
Length of output: 106
🏁 Script executed:
# Find the me-text-input file for comparison
fd -t f "me-text-input.ts" | head -5Repository: umekikazuya/me
Length of output: 108
🏁 Script executed:
# Read the me-textarea.ts file to check structure and CSS
cat -n frontend/src/components/admin/ui/me-textarea.tsRepository: umekikazuya/me
Length of output: 4750
🏁 Script executed:
# Read the me-text-input.ts file for comparison
cat -n frontend/src/components/admin/ui/me-text-input.tsRepository: umekikazuya/me
Length of output: 5664
プレースホルダーが空文字列の場合、:invalid:not(:placeholder-shown) は危険です。
このセレクターは、プレースホルダーがない（または空）要素では :placeholder-shown にマッチしないため、:not(:placeholder-shown) が常にマッチしてしまいます。その結果、ユーザー入力がない段階で必須項目がエラー表示になります。me-text-input も同じ問題を抱えています。
修正方法としては、すべての要件フィールドに placeholder=" " （スペース）を設定するか、:user-invalid 疑似クラス（2026年時点で広くサポート）に切り替えることを推奨します。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/admin/ui/me-textarea.ts` around lines 140 - 142, The
CSS selector textarea:invalid:not(:placeholder-shown) causes false-positive
error styling when the placeholder is absent or empty; update the code in
me-textarea (and the similar me-text-input) to either ensure required inputs
have a non-empty placeholder (e.g. placeholder=" ") so :placeholder-shown
behaves correctly, or replace the selector with the newer :user-invalid
pseudo-class (e.g. textarea:user-invalid) to target user-caused validation only;
change the selector and ensure both me-textarea and me-text-input use the same
approach for consistency.
Comment on lines
+166
to
176
| private async handleRouteChange() { | ||
| if (this.currentPath === '/admin/login') { | ||
| await this.auth.refreshSession() | ||
| return | ||
| } | ||
| if ( | ||
| changedProperties.has('currentPath') && | ||
| this.isAdminPath(this.currentPath) | ||
| this.isProtectedAdminPath(this.currentPath) && | ||
| this.auth.status === 'unknown' | ||
| ) { | ||
| void this.syncAdminRouteState() | ||
| await this.auth.refreshSession() | ||
| } |
Contributor
There was a problem hiding this comment.
guest のまま保護ルートへ入るケースが抜けています。
handleRouteChange() は status === 'unknown' のときしか保護ルートを処理しないため、すでに guest な状態で /admin/profile などへ直接遷移した場合は /admin/login へ飛ばず、me-auth-guard が nothing を返して空画面になります。ここでも guest を検知してログイン画面へリダイレクトしてください。
修正例
private async handleRouteChange() {
if (this.currentPath === '/admin/login') {
await this.auth.refreshSession()
return
}
+ if (
+ this.isProtectedAdminPath(this.currentPath) &&
+ this.auth.status === 'guest'
+ ) {
+ this.adminReturnPath = this.currentPath
+ await this.navigateToPath('/admin/login', true, true)
+ return
+ }
if (
this.isProtectedAdminPath(this.currentPath) &&
this.auth.status === 'unknown'
) {
await this.auth.refreshSession()
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/app-root.ts` around lines 166 - 176, The route
handler handleRouteChange currently only refreshes the session when auth.status
=== 'unknown' and therefore misses the case where auth.status === 'guest' trying
to access a protected admin path; update handleRouteChange to also detect when
this.currentPath is a protected admin path (use
isProtectedAdminPath(this.currentPath)) and auth.status === 'guest' and in that
case navigate/redirect to '/admin/login' (or otherwise trigger the same redirect
flow as the /admin/login branch) so requests to protected routes like
/admin/profile do not end up with me-auth-guard returning nothing.
Comment on lines
+22
to
+23
| host.addController(this) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "(RepositoryObserver|me-auth-guard)" | head -20Repository: umekikazuya/me
Length of output: 155
🏁 Script executed:
cat -n frontend/src/controllers/RepositoryObserver.tsRepository: umekikazuya/me
Length of output: 1871
🏁 Script executed:
cat -n frontend/src/components/admin/ui/me-auth-guard.tsRepository: umekikazuya/me
Length of output: 2517
🏁 Script executed:
rg "RepositoryObserver" --type ts --type tsx -A 3 -B 1Repository: umekikazuya/me
Length of output: 86
🏁 Script executed:
rg "disconnect\(\)" --type ts --type tsx frontend/src/components -B 2 -A 1 | head -50Repository: umekikazuya/me
Length of output: 86
🏁 Script executed:
rg "RepositoryObserver" -g "*.ts" -A 3 -B 1Repository: umekikazuya/me
Length of output: 9586
🏁 Script executed:
rg "new RepositoryObserver" -g "*.ts" -B 2 -A 2Repository: umekikazuya/me
Length of output: 3053
🏁 Script executed:
rg "_observer.*disconnect" -g "*.ts" -B 1 -A 1Repository: umekikazuya/me
Length of output: 1870
🏁 Script executed:
rg "router" -g "*.ts" frontend/src | grep -i "navigate\|route" | head -20Repository: umekikazuya/me
Length of output: 293
🏁 Script executed:
cat frontend/src/components/app-root.ts | head -80Repository: umekikazuya/me
Length of output: 2844
RepositoryObserver が host に登録されたまま残ります。
disconnect() はイベントリスナーを削除するだけで、Lit の controller として host から登録解除されません。repo 変更時に新しい observer を作成する実装（me-auth-guard.ts line 17-18 など複数箇所）では、旧 observer が host の controller リストに残存します。SPA ナビゲーション後に要素が再接続されると、Lit は登録済みの全 controller に hostConnected() を呼び出すため、旧 observer が古い repository に再購読し、更新通知が二重化します。
controller 自体を登録解除するか、observer を再利用して repository だけ切り替える設計に変更してください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/controllers/RepositoryObserver.ts` around lines 22 - 23, The
RepositoryObserver is left registered as a Lit controller
(host.addController(this)) and disconnect() only removes event listeners, so old
observers remain in the host's controller list and continue to receive
hostConnected(); fix by either unregistering the controller in your teardown or
switching to reusing the observer: add a host.removeController(this) call when
you intend to dispose the observer (e.g., inside RepositoryObserver.disconnect
or in the same place you remove listeners), or refactor RepositoryObserver to
expose a method like setRepository(newRepo) and update callers (e.g., places
creating new observers such as me-auth-guard.ts) to reuse a single observer and
call setRepository instead of instantiating/registering a new controller.
Comment on lines
+99
to
+115
| async login(input: AdminLoginInput) { | ||
| this._loginPending = true | ||
| this._loginError = '' | ||
| this.notifyChange() | ||
| try { | ||
| await apiLogin(input) | ||
| this._status = 'authenticated' | ||
| this._loginNotice = '' | ||
| this._loginError = '' | ||
| this.dispatchStatusChange() | ||
| } catch (error) { | ||
| this._loginError = describeApiError(error) | ||
| this.notifyChange() | ||
| } finally { | ||
| this._loginPending = false | ||
| this.notifyChange() | ||
| } |
Contributor
There was a problem hiding this comment.
refreshSession() の遅延結果が新しい認証 state を巻き戻します。
/admin/login では自動 refreshSession() と手動 login() が並行できます。login() が先に成功しても、先行していた refreshSession() の 401/失敗が後から返ると Line 149-151 で guest に戻され、成功ログイン直後に再びログイン画面へ戻されます。refresh の適用前に世代/token を確認するか、別の認証操作が始まった時点で進行中 refresh の結果を無効化してください。
Also applies to: 138-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/domain/AuthRepository.ts` around lines 99 - 115, Concurrent
refreshSession() results can overwrite a newer auth state set by login(); to
fix, add a generation/version or token check to ignore stale refresh results:
increment a private authGeneration (or snapshot current token) at the start of
any top-level auth operation (e.g. login()) and have refreshSession() capture
that generation/token before awaiting; when the refresh result resolves, compare
the captured generation/token to the current one and only apply changes to
_status/_loginError/_loginNotice if they match, otherwise discard the stale
result. Ensure the same guard is used where refreshSession() updates state so
any in-flight refresh is invalidated when another auth op begins.
Comment on lines
+175
to
+187
| async changeEmail(input: ChangeEmailInput) { | ||
| this._accountBusyAction = 'change-email' | ||
| this.notifyChange() | ||
| try { | ||
| await apiChangeEmail(input) | ||
| this._accountSuccess = 'メールアドレス変更を送信しました。' | ||
| this.notifyChange() | ||
| } catch (error) { | ||
| this._accountError = describeApiError(error) | ||
| this.notifyChange() | ||
| } finally { | ||
| this._accountBusyAction = '' | ||
| this.notifyChange() |
Contributor
There was a problem hiding this comment.
changeEmail() が前回の結果メッセージを残したままです。
開始時に _accountError と _accountSuccess をクリアしていないので、前回成功後の失敗で成功/失敗メッセージが同時に残ります。新しい試行の先頭で両方を空にしてください。
修正例
async changeEmail(input: ChangeEmailInput) {
this._accountBusyAction = 'change-email'
+ this._accountError = ''
+ this._accountSuccess = ''
this.notifyChange()
try {
await apiChangeEmail(input)
this._accountSuccess = 'メールアドレス変更を送信しました。'
this.notifyChange()
} catch (error) {
this._accountError = describeApiError(error)
this.notifyChange()
} finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/domain/AuthRepository.ts` around lines 175 - 187, In
changeEmail(), clear previous result state at the start by setting
this._accountError = '' and this._accountSuccess = '' (then call notifyChange)
before setting this._accountBusyAction; this ensures old success/failure
messages don't persist into a new attempt while keeping the existing
try/catch/finally logic (refer to changeEmail, _accountError, _accountSuccess,
_accountBusyAction, notifyChange, apiChangeEmail).
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.
Summary