Feature/atcoder verified voting#3319
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAtCoder認証フラグをサーバーからページへ渡し、コンポーネントのプロップを flat から Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant PageServer as PageServer (load/actions)
participant App as Svelte Components
participant DB as Database
rect rgba(200,200,255,0.5)
Browser->>PageServer: GET /votes/[slug] (セッション)
PageServer->>DB: セッション/ユーザ検索
DB-->>PageServer: user { is_validated }
PageServer-->>Browser: page data { ..., isAtCoderVerified }
end
rect rgba(200,255,200,0.5)
Browser->>App: レンダリング（data を受け渡し）
App->>App: $derived(data.isAtCoderVerified)
App-->>Browser: 投票 UI を表示（許可 or 誘導）
end
rect rgba(255,200,200,0.5)
Browser->>PageServer: POST /vote action
PageServer->>DB: check session.user.is_validated
alt not validated
PageServer-->>Browser: fail(FORBIDDEN)
else validated
PageServer->>DB: 投票記録
PageServer-->>Browser: success
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
認証（AtCoderアカウント検証）を投票機能から独立させつつ、未認証ユーザーの投票をUI/サーバー両面で制限する変更です。投票導線からプロフィール編集（AtCoder認証タブ）へ誘導できるようにしています。
Changes:
- 投票（詳細ページ/一覧のドロップダウン投票）を「ログイン + AtCoder認証済み」に制限
- 未認証ユーザー向けに
/users/edit?tab=atcoderへの誘導UIを追加 - ユーザー編集画面でAtCoder認証タブを復帰し、
?tab=atcoderで初期表示タブを切替
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/routes/votes/[slug]/+page.svelte | 未認証ログインユーザーへ認証誘導UIを追加 |
| src/routes/votes/[slug]/+page.server.ts | loadに isAtCoderVerified を追加 |
| src/routes/users/edit/+page.svelte | AtCoder認証タブ復帰、?tab=atcoder と form 結果でタブ自動オープン |
| src/routes/users/edit/+page.server.ts | tab=atcoder をloadへ反映 |
| src/routes/problems/+page.svelte | TaskTableへ isAtCoderVerified を伝播 |
| src/routes/problems/+page.server.ts | loadに isAtCoderVerified を追加 |
| src/lib/constants/navbar-links.ts | EDIT_PROFILE_PAGE を追加 |
| src/lib/components/AtCoderUserValidationForm.svelte | バインド/状態管理を見直し、入力をローカルstate化 |
| src/features/votes/components/VotableGrade.svelte | 未認証時の投票UIを誘導に切替、isAtCoderVerified を追加 |
| src/features/votes/actions/vote_actions.ts | サーバー側で未認証投票を403で拒否 |
| src/features/tasks/components/contest-table/TaskTableBodyCell.svelte | isAtCoderVerified を VotableGradeへ伝播 |
| src/features/tasks/components/contest-table/TaskTable.svelte | isAtCoderVerified を子へ伝播 |
Comment on lines
+121
to
+132
| {:else if data.isLoggedIn && !data.isAtCoderVerified} | ||
| <!-- ログイン済み・未認証 → 認証誘導 --> | ||
| <div | ||
| class="bg-yellow-50 dark:bg-yellow-900/20 border border-yellow-200 dark:border-yellow-700 rounded-lg p-4 mb-4" | ||
| > | ||
| <p class="text-yellow-800 dark:text-yellow-200 font-medium mb-2"> | ||
| 投票するにはAtCoderアカウントの認証が必要です。 | ||
| </p> | ||
| <Button href={editProfileHref} color="yellow" size="sm">AtCoderアカウントを認証する</Button> | ||
| </div> | ||
| {:else if data.isLoggedIn} | ||
| <!-- 未投票・ログイン済み → 投票フォーム --> | ||
| <!-- 未投票・ログイン済み・認証済み → 投票フォーム --> |
There was a problem hiding this comment.
The new “logged in but unverified” branch only applies when data.myVote?.voted is false. If a user voted previously and later resets AtCoder verification, they will still hit the data.myVote?.voted branch and see the “投票を変更する” form, but the server action now rejects unverified users (403). Consider also gating the vote-change UI in the voted branch on data.isAtCoderVerified (or showing the same verification prompt there) to avoid a broken flow.
|
|
||
| // Editable only in 'nothing' step; server is authoritative after each action. | ||
| // untrack: prop is the initial seed only — intentional one-time capture. | ||
| let editableAtcoderId = $state(untrack(() => atcoder_username)); | ||
|
|
There was a problem hiding this comment.
editableAtcoderId is seeded once from atcoder_username and never re-synced. After a successful reset (server clears atcoder_username and status becomes nothing), the input will still show the old AtCoder ID, diverging from server-authoritative state. Consider syncing editableAtcoderId when status becomes nothing (or when atcoder_username changes to an empty string) while still avoiding overwriting during user edits.
Suggested change
| // Keep editableAtcoderId in sync with server after a reset: | |
| // when the server clears atcoder_username and status returns to 'nothing', | |
| // reset the local editable value as well. | |
| $effect(() => { | |
| if (status === 'nothing' && atcoder_username === '') { | |
| editableAtcoderId = ''; | |
| } | |
| }); |
Comment on lines
+31
to
+35
| if (!locals.user?.is_validated) { | ||
| return fail(FORBIDDEN, { | ||
| message: 'AtCoderアカウントの認証が必要です。', | ||
| }); | ||
| } |
There was a problem hiding this comment.
The verification check conflates “user is not AtCoder-verified” with “locals.user is missing”. Since locals.user is populated in hooks.server.ts by looking up the DB user, a deleted/missing user could yield locals.user === undefined even when session is valid; returning 403 with an AtCoder-verification message would be misleading. Consider explicitly handling !locals.user (e.g., treat it as UNAUTHORIZED/INTERNAL_SERVER_ERROR) and only return FORBIDDEN when the user exists but is_validated is false.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ting Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Eliminates 4 identical username-read + requireSelf blocks across generate/validate/reset/delete actions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/problems/`+page.svelte:
- Line 27: Replace the plain assignments from the load() store with derived
stores so they update when data reloads: import { derived } from 'svelte/store'
and change the variables isAdmin, isLoggedIn, isAtCoderVerified, and voteResults
to derived(data, d => d.isAdmin), derived(data, d => d.isLoggedIn),
derived(data, d => d.isAtCoderVerified), and derived(data, d => d.voteResults)
respectively (leave taskResults as-is since it is already derived); reference
the top-level store named data and the variable names isAdmin, isLoggedIn,
isAtCoderVerified, voteResults, and taskResults when making the edits.
In `@src/routes/users/edit/`+page.svelte:
- Around line 27-30: The page currently only reads data.* for alerts so action
results (validate/reset/delete) that return message_type/message are ignored;
update the component to prefer the action result contained in the server form
payload (use the existing let { data, form }: Props = $props() and render
form?.message_type / form?.message when present), fall back to data.* only if
form is null/empty, and move the tab open/close logic into a shared util (e.g.,
a new helper in utils/ that takes the action result or form and returns which
tab should be active) so the component only renders based on that util and
contains no business logic in its <script> block. Ensure the server action names
validate/reset/delete are used as keys when reading form to keep behavior
consistent with +page.server.ts.
In `@src/routes/votes/`[slug]/+page.svelte:
- Around line 121-133: The branch order allows data.myVote?.voted to render the
vote-change form before checking AtCoder verification, so move the
isAtCoderVerified check earlier: ensure the {:else if data.isLoggedIn &&
!data.isAtCoderVerified} branch appears before any branch that evaluates
data.myVote?.voted (and keep the {:else if data.isLoggedIn} branch for the
voting form after the verification check). Update the Svelte conditionals so
data.isAtCoderVerified is required before rendering the data.myVote?.voted form
(adjust the blocks that reference data.myVote?.voted, data.isLoggedIn, and
data.isAtCoderVerified accordingly).
🪄 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: d8d87cb3-4865-4d92-8e66-44840e31ed6c
📒 Files selected for processing (14)
.claude/rules/svelte-components.md.claude/rules/sveltekit.mdsrc/features/account/components/settings/AtCoderVerificationForm.sveltesrc/features/tasks/components/contest-table/TaskTable.sveltesrc/features/tasks/components/contest-table/TaskTableBodyCell.sveltesrc/features/votes/actions/vote_actions.tssrc/features/votes/components/VotableGrade.sveltesrc/lib/constants/navbar-links.tssrc/routes/problems/+page.server.tssrc/routes/problems/+page.sveltesrc/routes/users/edit/+page.server.tssrc/routes/users/edit/+page.sveltesrc/routes/votes/[slug]/+page.server.tssrc/routes/votes/[slug]/+page.svelte
Comment on lines
+27
to
+30
| form: Record<string, unknown> | null; | ||
| } | ||
|
|
||
| let { data }: Props = $props(); | ||
| // TODO: Restore when AtCoderUserValidationForm is re-enabled (svelte/valid-prop-names-in-kit-pages requires $bindable props to be declared in Props): | ||
| // interface Props { ...; status?: string; } | ||
| // let { data, status = $bindable('nothing') }: Props = $props(); | ||
| // let status = $state('nothing'); | ||
| let { data, form }: Props = $props(); |
Contributor
There was a problem hiding this comment.
action の返却メッセージが画面に出ません
src/routes/users/edit/+page.server.ts の validate / reset / delete は message_type / message を action result で返していますが、このページは alert 用に data.* しか参照していません。load() は空文字を返しているので、本人確認失敗やリセット成功が無通知になります。form はタブ判定だけでなく表示メッセージにも使ってください。あわせて、タブ開閉判定は action 結果と密結合なので、utils/ 側へ寄せた方が分岐を揃えやすいです。
差分案
let role = data.role;
let username = data.username;
- let message = data.message;
- let message_type = data.message_type;
+ let message = $derived(typeof form?.message === 'string' ? form.message : data.message);
+ let message_type = $derived(
+ typeof form?.message_type === 'string' ? form.message_type : data.message_type,
+ );As per coding guidelines, "Business logic belongs in utils/, not inside <script> blocks."
Also applies to: 32-35, 39-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/users/edit/`+page.svelte around lines 27 - 30, The page currently
only reads data.* for alerts so action results (validate/reset/delete) that
return message_type/message are ignored; update the component to prefer the
action result contained in the server form payload (use the existing let { data,
form }: Props = $props() and render form?.message_type / form?.message when
present), fall back to data.* only if form is null/empty, and move the tab
open/close logic into a shared util (e.g., a new helper in utils/ that takes the
action result or form and returns which tab should be active) so the component
only renders based on that util and contains no business logic in its <script>
block. Ensure the server action names validate/reset/delete are used as keys
when reading form to keep behavior consistent with +page.server.ts.
Clarify that the URL is for the NoviSteps organization crawler. See team documentation for the actual endpoint. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 @.env.example:
- Around line 1-2: Restore the real AtCoder affiliation confirmation API URL
into the placeholder in .env.example (replace the placeholder comment with the
actual URL string value for the environment variable used by the crawler), and
update the accompanying comment to point to a specific team document or path
where the URL is documented; if the real URL is sensitive and appears in git
history, consider removing it from history or rotating credentials before
committing.
🪄 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: 4c32874f-1df2-46ec-a7cb-38a72d43f84b
📒 Files selected for processing (1)
.env.example
river0525
force-pushed
the
feature/atcoder-verified-voting
branch
from
5c3515d to
b8fd4ff
Compare
- +page.server.ts: wrap service calls in try/catch; return fail(500) on error - +page.server.ts: rename is_tab_atcoder -> isTabAtcoder (camelCase consistency) - +page.server.ts: import INTERNAL_SERVER_ERROR constant - +page.svelte (users/edit): use $derived for role/username/message/message_type - +page.svelte (users/edit): update form?.isTabAtcoder reference - problems/+page.svelte: use $derived for isAdmin/isLoggedIn/isAtCoderVerified/voteResults - votes/[slug]/+page.svelte: remove @ts-expect-error; append ?tab=atcoder after resolve() - e2e/votes.spec.ts: wait for vote form or unverified message before isVisible() check - AtCoderVerificationForm.svelte: consolidate imports at top of script block - .claude/rules/sveltekit.md: soften "never" to "typically" for undefined handling - .claude/rules/testing-e2e.md: allow type-only imports; add multi-test parameterized pattern - .claude/rules/testing.md: clarify "basic conditionals" with examples Skip: locals.user?.is_validated is correct — session.user type does not include is_validated Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- +page.server.ts: rename is_validated -> isValidated (camelCase consistency) - e2e/votes.spec.ts: reuse voteForm locator variable instead of redefining - e2e/custom_colors.spec.ts: make xs breakpoint regex flexible for spaces around >= - .claude/rules/testing-e2e.md: "URL string values" -> "string constant values"; fix gradeButton(grade) -> gradeButton(page, grade) for consistent signature - .claude/rules/testing.md: raise branch coverage target to 80%; clarify "real side effects" definition; require unit tests by default for components Skip: locals.user?.is_validated changes (session.user type lacks is_validated; locals.user is the correct authoritative source for AtCoder verification status) Skip: problems/+page.svelte $derived removal (contradicts project rule and round 1 fix) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/votes.spec.ts`:
- Around line 159-162: The test "non-admin user is redirected to /" expects the
wrong URL; since validateAdminAccess redirects non-admins to /login, update the
test in votes.spec.ts (the test using loginAsUser and VOTE_MANAGEMENT_URL) to
assert page navigates to '/login' (use the same TIMEOUT constant and page
helpers) OR alternatively change validateAdminAccess to redirect to '/' so
behavior matches the test—pick one approach and make the expectation and the
validateAdminAccess redirect consistent.
In `@src/routes/users/edit/`+page.server.ts:
- Around line 85-96: Rename the snake_case variable is_validated to camelCase
isValidated throughout the handler that calls verificationService.validate
(replace const { username } = parsed; const is_validated = await
verificationService.validate(username); and all subsequent uses in the returned
object) so naming matches other variables like
validationCode/formData/isTabAtcoder; update the returned property names
(success, message_type, message) to reference isValidated instead of
is_validated and preserve the same boolean logic and messages.
🪄 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: f7b0a9ce-9fa8-4e2d-928a-162e2ce7d427
📒 Files selected for processing (10)
.claude/rules/sveltekit.md.claude/rules/testing-e2e.md.claude/rules/testing.md.env.examplee2e/votes.spec.tssrc/features/account/components/settings/AtCoderVerificationForm.sveltesrc/routes/problems/+page.sveltesrc/routes/users/edit/+page.server.tssrc/routes/users/edit/+page.sveltesrc/routes/votes/[slug]/+page.svelte
- VotableGrade.svelte: remove @ts-expect-error; append ?tab=atcoder after resolve() - e2e/votes.spec.ts: add clarifying comment on intentional voteForm visibility check - .claude/rules/testing.md: rename cleanup subsection to "Integration Tests and Tests with Real Side Effects"; add note that it does not apply to Prisma-mock unit tests Skip: locals.user?.is_validated -> session.user (session.user type lacks is_validated) Skip: sitemap.xml /votes/[slug] exclusion (defensive pattern, not harmful) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
KATO-Hiro
approved these changes
Mar 29, 2026
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
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.
認証機能を分離したPRです。
サーバーの用意後、マージいたします。
Summary by CodeRabbit
新機能
表示・動作
ドキュメント
その他