refactor: 仕様書に基づいてコンポーネントを再設計#90
Merged
Merged
Conversation
Contributor
Walkthroughページ読み込み前にルートの Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
frontend/src/components/app-admin-shell.ts (1)
39-45:⚠️ Potential issue | 🟡 Minor認証確認中のステータスが二重表示されます。
Line 41-43 のメッセージを出しつつ Line 45 で slot も常に描画しているため、保護ルートでは
<me-auth-guard>側のローディング表示と重なります。待機中 UI はどちらか一方に寄せた方が自然です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/app-admin-shell.ts` around lines 39 - 45, The status message and the default slot are both rendered causing duplicate loading UIs; update the render logic in the component that uses isChecking (the template around main/outlet and the isChecking property) so that when isChecking is true you render only the status paragraph and do not render the <slot> (i.e., conditionally render the slot only when !this.isChecking) so the <me-auth-guard> loading UI and this component's loading UI don't overlap.frontend/src/domain/AuthRepository.ts (2)
175-189:⚠️ Potential issue | 🟡 Minor
changeEmail()も同様に_accountErrorと_accountSuccessをクリアしていない
revokeAllSessions()と同様に、changeEmail()も操作開始時にエラー/成功メッセージをクリアしていません。🐛 修正案
async changeEmail(input: ChangeEmailInput) { this._accountBusyAction = 'change-email' + this._accountError = '' + this._accountSuccess = '' this.notifyChange()🤖 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 - 189, The changeEmail method fails to clear prior status messages at start; update changeEmail (in AuthRepository) to reset this._accountError = '' and this._accountSuccess = '' (same approach used in revokeAllSessions) before setting this._accountBusyAction and calling notifyChange, so previous error/success messages don't persist across operations.
159-172:⚠️ Potential issue | 🟡 Minor
revokeAllSessions()で_accountErrorと_accountSuccessがクリアされていない
logout()（Lines 120-121）では操作開始時に_accountErrorと_accountSuccessをクリアしていますが、revokeAllSessions()ではクリアしていません。以前の操作からのメッセージが残る可能性があります。また、
logout()は成功時に_loginNoticeを設定していますが、revokeAllSessions()は設定していません。これは意図的な違いでしょうか？🐛 一貫性のための修正案
async revokeAllSessions() { this._accountBusyAction = 'revoke-sessions' + this._accountError = '' + this._accountSuccess = '' this.notifyChange() try { await apiRevokeAllSessions() this._status = 'guest' + this._loginNotice = 'すべてのセッションを無効化しました。' + this._accountSuccess = 'すべてのセッションを無効化しました。' this.dispatchStatusChange() } catch (error) {🤖 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 159 - 172, revokeAllSessions() currently doesn't clear prior status messages or set the same success notice as logout; update revokeAllSessions to clear this._accountError and this._accountSuccess at the start (same as logout()), and on successful completion set this._loginNotice the same way logout() does (or explicitly decide and document the difference), then notifyChange/dispatchStatusChange as appropriate so stale messages aren't shown; refer to the revokeAllSessions and logout methods and the fields _accountError, _accountSuccess, and _loginNotice when making the change.frontend/src/domain/ProfileRepository.ts (1)
89-119: 🧹 Nitpick | 🔵 Trivial
notifyChange()の重複呼び出しによる冗長なイベント発行
notifyPublicChange()は内部でnotifyChange()を呼び出していますが、loadPublicProfile()のfinallyブロック（Line 118）でもnotifyChange()が呼ばれます。成功時に'change'イベントが2回発行されます。同様のパターンが
loadAdminProfile()とsaveAdminProfile()にも存在します。saveAdminProfile()の成功時はnotifyAdminChange()+notifyPublicChange()+finallyで3回のnotifyChange()呼び出しになります。
RepositoryObserverが複数回の再描画をトリガーする可能性があります。♻️ 提案: finallyブロックでの重複呼び出しを回避
async loadPublicProfile() { if (this._publicProfile || this._publicLoading) return this._publicLoading = true this.notifyChange() try { await this._internalFetch() + this._publicLoading = false this.notifyPublicChange() } catch { // Fallback handled by components + this._publicLoading = false + this.notifyChange() - } finally { - this._publicLoading = false - this.notifyChange() } }🤖 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 89 - 119, notifyPublicChange() and notifyAdminChange() each call notifyChange() causing duplicate 'change' events when callers (loadPublicProfile, loadAdminProfile, saveAdminProfile) also call notifyChange() in their finally blocks; fix by removing the internal notifyChange() calls from notifyPublicChange() and notifyAdminChange() (so those methods only dispatch their specific CustomEvent), and ensure callers invoke notifyChange() exactly once after successful/failed flows (keep the finally-based notifyChange() or explicitly call notifyChange() after calling notifyPublicChange()/notifyAdminChange() where necessary) to prevent multiple redundant re-renders.
🤖 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/index.html`:
- Around line 15-22: The FOUC prevention script sets data-theme from
localStorage or prefers-color-scheme, but AppRoot
(frontend/src/components/app-root.ts — the AppRoot component) later overwrites
document.documentElement.setAttribute('data-theme') based on the URL path,
causing a flash; update the inline script to use the exact same path-based logic
as AppRoot to choose 'public' vs 'admin' (i.e., inspect location.pathname and
apply the same predicate AppRoot uses) and set
document.documentElement.setAttribute('data-theme', 'public'|'admin')
accordingly so the initial attribute matches AppRoot at load time.
In `@frontend/src/components/admin/ui/me-auth-guard.ts`:
- Around line 13-19: The authRepo setter only swaps the RepositoryObserver and
doesn't re-run session validation, so when a new IAuthRepository with state
'unknown' is injected the guard can remain stuck; update the authRepo setter
(the set authRepo(repo: IAuthRepository) method) so after assigning
this._authRepo and creating/replacing this._observer (RepositoryObserver) you
also invoke void this.checkSession() (or await if appropriate) to trigger
immediate session re-check; ensure you still disconnect the old observer
(this._observer?.disconnect()) before creating the new one.
In `@frontend/src/components/admin/ui/me-select.ts`:
- Around line 26-29: The formResetCallback currently forces an empty selection
by setting this.value = '' which prevents restoring the original selected
option; capture and persist the initial/default value when the component is
initialized (e.g., in the constructor or connectedCallback into a field like
this._initialValue) and then change formResetCallback to restore that stored
value (e.g., this.value = this._initialValue) before calling
this._syncInternals(); keep the existing symbols formResetCallback, this.value,
and this._syncInternals to locate and update the code.
In `@frontend/src/components/admin/ui/me-text-input.ts`:
- Around line 44-46: formResetCallback currently forces this.value to '' so
form.reset() cannot restore the component's initial/default value; change it to
restore a stored initial value (e.g., set this.value = this._initialValue) and
keep the existing this._syncInternals() call. Ensure you capture and store that
initial value when the element is created/connected (in the constructor or
connectedCallback) by reading the initial this.value or the initial attribute
(e.g., this.getAttribute('value') or a defaultValue prop) into
this._initialValue so formResetCallback and other logic can restore the original
default.
In `@frontend/src/components/admin/ui/me-textarea.ts`:
- Around line 31-33: The formResetCallback currently forces this.value = ''
which breaks restoring initial values on form.reset(); change formResetCallback
(in me-textarea) to restore the saved initial/default value (the same mechanism
used by me-text-input and me-select) instead of hardcoding empty string — e.g.
set this.value = this._initialValue (or this._defaultValue / the stored default
you use elsewhere) and then call this._syncInternals(); ensure the initial value
is captured at construction/connectedCallback so reset can revert to it.
In `@frontend/src/components/app-root.ts`:
- Around line 169-175: handleRouteChange currently calls auth.refreshSession
only for routes where isProtectedAdminPath(this.currentPath) is true, so
navigating directly to "/admin/login" (not wrapped by me-auth-guard) skips
session recheck; update handleRouteChange to also call await
this.auth.refreshSession() when this.currentPath === '/admin/login' and
this.auth.status === 'unknown' (in addition to the existing protected-route
check) so that auth.status is populated and authenticated users get redirected;
locate the method handleRouteChange and the use of isProtectedAdminPath,
currentPath, and auth.refreshSession/auth.status to implement this extra
conditional.
In `@frontend/src/domain/AuthRepository.ts`:
- Around line 90-97: dispatchStatusChange currently calls notifyChange and
callers like login, logout, revokeAllSessions also call notifyChange in their
finally blocks, causing duplicate notifications; follow the refreshSession
pattern instead: remove the notifyChange() call from dispatchStatusChange()
(leave it to only dispatch the CustomEvent) and keep the existing notifyChange()
calls in the callers (login, logout, revokeAllSessions) so each status change
results in a single notifyChange invocation; update references to
dispatchStatusChange(), login(), logout(), revokeAllSessions(), and
refreshSession() accordingly.
---
Outside diff comments:
In `@frontend/src/components/app-admin-shell.ts`:
- Around line 39-45: The status message and the default slot are both rendered
causing duplicate loading UIs; update the render logic in the component that
uses isChecking (the template around main/outlet and the isChecking property) so
that when isChecking is true you render only the status paragraph and do not
render the <slot> (i.e., conditionally render the slot only when
!this.isChecking) so the <me-auth-guard> loading UI and this component's loading
UI don't overlap.
In `@frontend/src/domain/AuthRepository.ts`:
- Around line 175-189: The changeEmail method fails to clear prior status
messages at start; update changeEmail (in AuthRepository) to reset
this._accountError = '' and this._accountSuccess = '' (same approach used in
revokeAllSessions) before setting this._accountBusyAction and calling
notifyChange, so previous error/success messages don't persist across
operations.
- Around line 159-172: revokeAllSessions() currently doesn't clear prior status
messages or set the same success notice as logout; update revokeAllSessions to
clear this._accountError and this._accountSuccess at the start (same as
logout()), and on successful completion set this._loginNotice the same way
logout() does (or explicitly decide and document the difference), then
notifyChange/dispatchStatusChange as appropriate so stale messages aren't shown;
refer to the revokeAllSessions and logout methods and the fields _accountError,
_accountSuccess, and _loginNotice when making the change.
In `@frontend/src/domain/ProfileRepository.ts`:
- Around line 89-119: notifyPublicChange() and notifyAdminChange() each call
notifyChange() causing duplicate 'change' events when callers
(loadPublicProfile, loadAdminProfile, saveAdminProfile) also call notifyChange()
in their finally blocks; fix by removing the internal notifyChange() calls from
notifyPublicChange() and notifyAdminChange() (so those methods only dispatch
their specific CustomEvent), and ensure callers invoke notifyChange() exactly
once after successful/failed flows (keep the finally-based notifyChange() or
explicitly call notifyChange() after calling
notifyPublicChange()/notifyAdminChange() where necessary) to prevent multiple
redundant re-renders.
🪄 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: 30d9f106-db00-4749-b876-2b899ebc1d1f
📒 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
Comment on lines
+13
to
+19
| @consume({ context: authContext, subscribe: true }) | ||
| set authRepo(repo: IAuthRepository) { | ||
| if (this._authRepo === repo) return | ||
| this._authRepo = repo | ||
| this._observer?.disconnect() | ||
| if (repo) this._observer = new RepositoryObserver(this, repo) | ||
| } |
Contributor
There was a problem hiding this comment.
authRepo の差し替え時に checkSession() が再実行されません。
この setter は observer を張り替えるだけなので、新しい repo が 'unknown' のまま入ってきたケースを取りこぼします。差し替え直後にも void this.checkSession() を呼んでおかないと、ガードが空描画のまま止まる可能性があります。
修正案
set authRepo(repo: IAuthRepository) {
if (this._authRepo === repo) return
this._authRepo = repo
this._observer?.disconnect()
- if (repo) this._observer = new RepositoryObserver(this, repo)
+ if (repo) {
+ this._observer = new RepositoryObserver(this, repo)
+ void this.checkSession()
+ }
}📝 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
| @consume({ context: authContext, subscribe: true }) | |
| set authRepo(repo: IAuthRepository) { | |
| if (this._authRepo === repo) return | |
| this._authRepo = repo | |
| this._observer?.disconnect() | |
| if (repo) this._observer = new RepositoryObserver(this, repo) | |
| } | |
| `@consume`({ context: authContext, subscribe: true }) | |
| set authRepo(repo: IAuthRepository) { | |
| if (this._authRepo === repo) return | |
| this._authRepo = repo | |
| this._observer?.disconnect() | |
| if (repo) { | |
| this._observer = new RepositoryObserver(this, repo) | |
| void this.checkSession() | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/admin/ui/me-auth-guard.ts` around lines 13 - 19, The
authRepo setter only swaps the RepositoryObserver and doesn't re-run session
validation, so when a new IAuthRepository with state 'unknown' is injected the
guard can remain stuck; update the authRepo setter (the set authRepo(repo:
IAuthRepository) method) so after assigning this._authRepo and
creating/replacing this._observer (RepositoryObserver) you also invoke void
this.checkSession() (or await if appropriate) to trigger immediate session
re-check; ensure you still disconnect the old observer
(this._observer?.disconnect()) before creating the new one.
Comment on lines
26
to
29
| formResetCallback() { | ||
| this.value = '' | ||
| this._internals.setFormValue('') | ||
| this._syncInternals() | ||
| } |
Contributor
There was a problem hiding this comment.
formResetCallback() が空選択へ固定されています。
Line 27 で常に '' を入れると、初期選択があるフォームでも reset 後に元の option へ戻りません。ここも既定値へ戻す実装にしてください。
🤖 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 26 - 29, The
formResetCallback currently forces an empty selection by setting this.value = ''
which prevents restoring the original selected option; capture and persist the
initial/default value when the component is initialized (e.g., in the
constructor or connectedCallback into a field like this._initialValue) and then
change formResetCallback to restore that stored value (e.g., this.value =
this._initialValue) before calling this._syncInternals(); keep the existing
symbols formResetCallback, this.value, and this._syncInternals to locate and
update the code.
Comment on lines
44
to
+46
| formResetCallback() { | ||
| this.value = '' | ||
| this._internals.setFormValue('') | ||
| this._syncInternals() |
Contributor
There was a problem hiding this comment.
formResetCallback() が初期値ではなく空文字へ固定されています。
Line 45 で常に '' を代入しているため、初期値付きのフォームでも form.reset() が元の値へ戻りません。ここは空文字固定ではなく、コンポーネントの既定値へ戻す必要があります。
🤖 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 44 - 46,
formResetCallback currently forces this.value to '' so form.reset() cannot
restore the component's initial/default value; change it to restore a stored
initial value (e.g., set this.value = this._initialValue) and keep the existing
this._syncInternals() call. Ensure you capture and store that initial value when
the element is created/connected (in the constructor or connectedCallback) by
reading the initial this.value or the initial attribute (e.g.,
this.getAttribute('value') or a defaultValue prop) into this._initialValue so
formResetCallback and other logic can restore the original default.
Comment on lines
31
to
+33
| formResetCallback() { | ||
| this.value = '' | ||
| this._internals.setFormValue('') | ||
| this._syncInternals() |
Contributor
There was a problem hiding this comment.
textarea も reset 時に初期値へ戻っていません。
Line 32 が空文字固定なので、既定値付きフォームで form.reset() しても初期内容を復元できません。me-text-input/me-select と同様に、ここも保存している既定値へ戻す必要があります。
🤖 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 31 - 33, The
formResetCallback currently forces this.value = '' which breaks restoring
initial values on form.reset(); change formResetCallback (in me-textarea) to
restore the saved initial/default value (the same mechanism used by
me-text-input and me-select) instead of hardcoding empty string — e.g. set
this.value = this._initialValue (or this._defaultValue / the stored default you
use elsewhere) and then call this._syncInternals(); ensure the initial value is
captured at construction/connectedCallback so reset can revert to it.
Comment on lines
+90
to
+97
| private dispatchStatusChange() { | ||
| this.dispatchEvent( | ||
| new CustomEvent('auth:status-change', { | ||
| detail: { status: this._status }, | ||
| }), | ||
| ) | ||
| this.notifyChange() | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
ProfileRepositoryと同様のnotifyChange()重複呼び出し
dispatchStatusChange()が内部でnotifyChange()を呼び出していますが、login()のfinallyブロック（Line 114）でもnotifyChange()が呼ばれます。logout()、revokeAllSessions()も同様です。
一方、refreshSession()はfinallyブロックでnotifyChange()を呼ばないため、重複がありません。これを他のメソッドの参考パターンにできます。
Also applies to: 99-116
🤖 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 90 - 97,
dispatchStatusChange currently calls notifyChange and callers like login,
logout, revokeAllSessions also call notifyChange in their finally blocks,
causing duplicate notifications; follow the refreshSession pattern instead:
remove the notifyChange() call from dispatchStatusChange() (leave it to only
dispatch the CustomEvent) and keep the existing notifyChange() calls in the
callers (login, logout, revokeAllSessions) so each status change results in a
single notifyChange invocation; update references to dispatchStatusChange(),
login(), logout(), revokeAllSessions(), and refreshSession() accordingly.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/src/components/app-root.ts (1)
166-176:⚠️ Potential issue | 🟠 Major
/admin/loginの再確認にrefreshSession()をそのまま使うと 401 通知の副作用が出ます。
frontend/src/domain/AuthRepository.ts:138-157のrefreshSession()は 401 でstatus = 'guest'だけでなくloginNotice = 'セッションが切れました。'も設定します。ここの無条件呼び出しだと、未ログインで/admin/loginを開いただけのケースやログアウト直後の遷移でも、不要な再確認と期限切れ通知が発生します。ログインページのブートストラップは silent な再確認に分けるか、最低でもstatus === 'unknown'のときだけ呼ぶようにしてください。最小修正案
private async handleRouteChange() { - if (this.currentPath === '/admin/login') { + if ( + this.currentPath === '/admin/login' && + this.auth.status === 'unknown' + ) { await this.auth.refreshSession() return }🤖 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 current handleRouteChange calls auth.refreshSession() unconditionally for '/admin/login', but AuthRepository.refreshSession() sets loginNotice on 401, causing unwanted "session expired" notices when simply opening the login page; change handleRouteChange so it only triggers a full refresh when auth.status === 'unknown' or else call a silent recheck method instead (e.g. implement and call auth.silentRefreshSession() that mirrors AuthRepository.refreshSession() without setting loginNotice on 401). Update the logic in handleRouteChange (and add the silent method to the auth API if missing) so '/admin/login' bootstraps via a silent check or is gated by auth.status === 'unknown' to prevent spurious notices.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/components/app-root.ts`:
- Around line 166-176: The current handleRouteChange calls auth.refreshSession()
unconditionally for '/admin/login', but AuthRepository.refreshSession() sets
loginNotice on 401, causing unwanted "session expired" notices when simply
opening the login page; change handleRouteChange so it only triggers a full
refresh when auth.status === 'unknown' or else call a silent recheck method
instead (e.g. implement and call auth.silentRefreshSession() that mirrors
AuthRepository.refreshSession() without setting loginNotice on 401). Update the
logic in handleRouteChange (and add the silent method to the auth API if
missing) so '/admin/login' bootstraps via a silent check or is gated by
auth.status === 'unknown' to prevent spurious notices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: abe0c725-a12f-4e30-9b7c-c7fc77c8bdc9
📒 Files selected for processing (2)
frontend/index.htmlfrontend/src/components/app-root.ts
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