Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
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:
📝 WalkthroughWalkthroughバックエンドで paper の可視性検証と orgIds 処理を追加し、GET が org_only の場合に organizations を返すように変更。フロントエンドは編集画面で組織データを並列取得・表示し、フォームで org 選択と送信バリデーションを行う。テスト群を対応して拡張。 Changes
Sequence DiagramssequenceDiagram
participant Client as Client
participant API as API
participant DB as DB
Client->>API: GET /api/papers/:id
activate API
API->>DB: SELECT paper by id
activate DB
DB-->>API: paper
deactivate DB
alt paper.visibility == "org_only"
API->>DB: SELECT organizations via paperOrgs join
activate DB
DB-->>API: organizations[]
deactivate DB
else
API->>API: organizations = []
end
API-->>Client: { paper, authors, organizations }
deactivate API
sequenceDiagram
participant Client as Client
participant API as API
participant DB as DB
Client->>API: PATCH /api/papers/:id { visibility, orgIds? }
activate API
API->>DB: SELECT paper by id
activate DB
DB-->>API: existing paper
deactivate DB
API->>API: validate nextVisibility, normalize orgIds
alt nextVisibility == "org_only" && orgIds provided
API->>DB: COUNT orgMembers WHERE user_id & org_id IN (orgIds)
activate DB
DB-->>API: count
deactivate DB
alt count != orgIds.length
API-->>Client: 403 Forbidden
else
API->>DB: batch([ UPDATE paper, DELETE paperOrgs, INSERT paperOrgs... ])
activate DB
DB-->>API: ok
deactivate DB
API-->>Client: 200 OK
end
else if nextVisibility != "org_only" && orgIds provided
API-->>Client: 400 Bad Request
else
API->>DB: UPDATE paper (no paperOrgs change)
API-->>Client: 200 OK
end
deactivate API
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! このプルリクエストは、org_only（組織内限定公開）の公開範囲を持つ論文について、作成から編集まで一貫して正しく扱えるようにするための包括的な機能強化を目的としています。これにより、組織内で共有される論文の管理がより柔軟かつ正確に行えるようになり、ユーザーは公開設定を細かく制御できるようになります。 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
このプルリクエストは、org_only公開範囲の論文の作成から編集までを完全にサポートするための変更を導入しています。API側では、org_only公開範囲の論文の取得時に所属組織情報を含めるようになり、PATCHエンドポイントでは、org_onlyへの変更時にorgIdsの検証と、ユーザーが指定されたすべての組織のメンバーであることの確認が追加されました。フロントエンドでは、論文編集ページでユーザーの所属組織をフェッチし、org_onlyを選択した場合に組織を選択できるUIが追加されています。また、関連するテストも追加・更新されており、新機能が適切に動作することを確認しています。全体的に、この機能はよく実装されており、必要な検証とUIの変更が適切に行われています。
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 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 `@apps/api/src/routes/papers.ts`:
- Line 1137: body.visibility is a plain string but nextVisibility is typed as
the union "public" | "org_only" | "private"; validate or narrow body.visibility
before assignment (e.g., check it is one of "public", "org_only", "private" and
then assign to nextVisibility), or map/convert the incoming string to the union
and handle invalid values (throw or set a safe default) so the TypeScript type
error is resolved while avoiding an unsafe cast on body.visibility.
In `@apps/web/src/app/papers/`[id]/edit/page.tsx:
- Around line 110-117: When orgsRes is not ok, instead of silently calling
setOrganizations([]) update the component to record and surface the error:
capture the failure (use orgsRes.status or await orgsRes.text()) and call a new
state setter (e.g. setOrgsError or setOrganizationsError) alongside
setOrganizations([]), log the response/error, and render a user-facing
notification (toast or inline alert) in the UI so users know org fetch failed;
update the code paths around orgsRes, setOrganizations, and the fetch/useEffect
that calls them to set and display this error state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 655716d7-96e6-46da-afda-cfb3bc1e5071
📒 Files selected for processing (4)
apps/api/src/routes/__tests__/papers.test.tsapps/api/src/routes/papers.tsapps/web/src/app/__tests__/paper-edit-page.test.tsxapps/web/src/app/papers/[id]/edit/page.tsx
…itization, and lint fixes)
- Fix visibility type narrowing and explicit orgIds guard - Avoid org JOIN on non-org_only paper detail fetch - Remove unreachable loadingOrgs logic and surface org fetch warning - Avoid unchanged orgIds submission from edit form - Add/adjust API and web tests for updated behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 `@apps/api/src/routes/papers.ts`:
- Around line 1164-1179: Add an explicit max-length check for each org ID before
accepting it into normalizedOrgIds: define a constant (e.g., MAX_ORG_ID_LEN) and
after trimming the orgId in the loop that processes body.orgIds (the block using
normalizedOrgIds, seen, and setting orgIdsFromBody) validate trimmed.length <=
MAX_ORG_ID_LEN, returning c.json({ error: "orgId exceeds maximum length" }, 400)
if it’s too long; perform this check before duplicate-checking/adding to seen so
overly long values are rejected early.
- Around line 1321-1329: The update to papers
(db.update(papers).set(updates).where(eq(papers.id, paperId))) and the
subsequent modifications to paperOrgs (db.delete / db.insert using paperOrgs,
finalOrgIds, paperId) must be executed atomically: wrap the papers update and
the conditional delete/insert logic (the branches using nextVisibility,
hasVisibilityInBody, shouldReplacePaperOrgs) inside a single DB transaction (use
your DB client's transaction API) so any failure rolls back both the papers
change and the paperOrgs changes; ensure you perform the delete when visibility
moves away from "org_only" and the replace insert when shouldReplacePaperOrgs is
true inside that same transaction and propagate/throw errors to trigger
rollback.
In `@apps/web/src/app/papers/`[id]/edit/page.tsx:
- Around line 193-204: The current guard for visibility === "org_only" blocks
updates even when the existing paper already has organization IDs; change it so
you only error when the user is trying to switch to org_only or actively change
the org selection: keep the first check but require both organizations.length
=== 0 AND initialSelectedOrgIds.length === 0 before blocking (so existing papers
with initialSelectedOrgIds can proceed), and add a second check that if
selectedOrgIds.length === 0 AND selectedOrgIds differs from
initialSelectedOrgIds then call setError (i.e., block only when the user
cleared/changed selection or there are no orgs available and no initial orgs).
Use the same symbols (visibility, organizations, selectedOrgIds,
initialSelectedOrgIds, setError) to locate and update the conditional logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f16a2cc-3255-461f-b43b-1bebd78bd32f
📒 Files selected for processing (8)
apps/api/src/middleware/__tests__/auth.test.tsapps/api/src/routes/__tests__/papers.test.tsapps/api/src/routes/__tests__/users.test.tsapps/api/src/routes/papers.tsapps/web/src/app/__tests__/paper-edit-page.test.tsxapps/web/src/app/orgs/[slug]/page.tsxapps/web/src/app/papers/[id]/edit/page.tsxapps/web/src/app/papers/[id]/page.tsx
Contributor
|
✅ Merge conflicts resolved successfully! Resolved 2 conflict file(s). Commit: 5 file operation(s)
View agent analysis |
Resolved conflicts in: - apps/web/src/app/orgs/[slug]/page.tsx (content) - apps/web/src/app/papers/[id]/page.tsx (content) Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
apps/api/src/routes/papers.ts (2)
1159-1179:⚠️ Potential issue | 🟡 Minor
orgIdsの各要素に長さ上限がありません。Line 1164-1176 では trim と重複排除だけなので、極端に長い ID が membership lookup と
paperOrgs更新候補まで進みます。ここでも org ID の最大長を先に弾いてください。As per coding guidelines:
apps/api/src/routes/**/*.ts: Validate all user-supplied strings with explicit length bounds before database writes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/papers.ts` around lines 1159 - 1179, The orgIds parsing block (orgIdsFromBody, normalizedOrgIds, seen) currently only trims and dedups but doesn't enforce a maximum length; add a check after trimming each orgId in the loop to reject values exceeding a defined max (e.g., MAX_ORG_ID_LENGTH) and return a 400 JSON error like the other validations; either reuse an existing MAX_ORG_ID_LENGTH constant or define one near the top of the file (or validation helpers) and use that constant in the loop to validate trimmed.length before adding to seen/normalizedOrgIds so overly long IDs never proceed to membership lookup or paperOrgs updates.
1321-1329:⚠️ Potential issue | 🟠 Major
papersとpaperOrgsの更新がまだ分離しています。Line 1321 で本体更新が確定した後に Line 1323-1329 の関連組織更新が別 await なので、後段で失敗すると
visibilityとpaper_orgsが不整合になります。特にorg_onlyへの変更途中で insert が失敗すると、論文だけがorg_onlyになり対象組織が欠けた状態が残ります。少なくともこの更新は同じ更新単位でまとめてください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/papers.ts` around lines 1321 - 1329, 現状、db.update(papers).set(updates).where(eq(papers.id, paperId)) と paperOrgs の delete/insert（paperOrgs, finalOrgIds, nextVisibility, hasVisibilityInBody, shouldReplacePaperOrgs）が別々の await になっており途中失敗で不整合になるため、これらを単一トランザクションにまとめてください。具体的には db.transaction(...) を使い、トランザクション内で先に db.update(papers).set(updates).where(eq(papers.id, paperId)) を実行し、その後の paperOrgs に対する delete と必要なら insert(finalOrgIds.map(...)) を同じトランザクション（tx.delete/tx.insert）で実行するように変更してください。apps/web/src/app/papers/[id]/edit/page.tsx (1)
193-204:⚠️ Potential issue | 🟠 Major既存の
org_only論文まで保存不能です。Line 194-197 が
organizations.length === 0だけで止めているため、data.organizationsで既存の選択を保持していても、所属組織取得失敗時や脱退後のタイトル更新までブロックされます。PATCH 側は未変更時のorgIds省略を許容しているので、未変更の組織選択を維持するケースは通してください。💡 最小修正案
- if (visibility === "org_only") { - if (organizations.length === 0) { + if (visibility === "org_only") { + const keepingExistingSelection = + initialSelectedOrgIds.length > 0 && + areSameOrgSelection(selectedOrgIds, initialSelectedOrgIds); + + if (organizations.length === 0 && !keepingExistingSelection) { setError("所属組織がありません。公開範囲を変更してください。"); return; } - if (selectedOrgIds.length === 0) { + if ( + selectedOrgIds.length === 0 && + !areSameOrgSelection(selectedOrgIds, initialSelectedOrgIds) + ) { setError( "組織公開にする場合は少なくとも1つの対象組織を選択してください。", );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/papers/`[id]/edit/page.tsx around lines 193 - 204, The guard in the save flow incorrectly blocks saving when visibility === "org_only" solely because organizations.length === 0, even if the paper already has organizations in data.organizations or the PATCH can omit unchanged orgIds; update the checks in the component handling visibility (use the variables visibility, organizations, selectedOrgIds and data.organizations) so that you only setError and return when there are no current org selections anywhere (i.e., organizations.length === 0 AND (data.organizations is empty or undefined) AND selectedOrgIds.length === 0); if data.organizations exists (non-empty) allow the save to proceed and omit orgIds from the PATCH, otherwise enforce selecting at least one organization and keep the existing error messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/__tests__/users.test.ts`:
- Around line 471-532: These three tests duplicate existing cases in the same
file (same test names and behavior found at lines ~332-395); remove the
duplicate tests or consolidate them into a single table-driven test to avoid
redundant coverage and noisy failures — locate the tests by their titles "GET
/api/users/me returns 404 when user is not found in database", "PATCH
/api/users/me rejects non-object body", and "PATCH /api/users/me rejects
displayName longer than 50 chars" and the helpers
createTestJWT/createTestApp/createTestEnv/mockDb used in them, then either
delete these duplicate blocks or refactor them into a parameterized test that
covers the distinct scenarios once.
In `@apps/web/src/app/papers/`[id]/edit/page.tsx:
- Around line 107-131: The Promise.all call currently bundles
apiFetch(`/api/papers/${encodeURIComponent(paperId)}`) and
apiFetch("/api/users/me/orgs") so a rejection from the orgs fetch aborts the
whole operation; separate the org fetch so paper fetch cannot fail due to org
errors—either replace Promise.all with Promise.allSettled and handle orgsRes
outcome separately, or keep the paper fetch in its own await and wrap
apiFetch("/api/users/me/orgs") in a try/catch; on org fetch failure
setOrganizations([]) and setOrgsWarning(...) but still allow the paperRes
handling (the existing paperRes.ok logic and router.replace call) to run
unaffected.
---
Duplicate comments:
In `@apps/api/src/routes/papers.ts`:
- Around line 1159-1179: The orgIds parsing block (orgIdsFromBody,
normalizedOrgIds, seen) currently only trims and dedups but doesn't enforce a
maximum length; add a check after trimming each orgId in the loop to reject
values exceeding a defined max (e.g., MAX_ORG_ID_LENGTH) and return a 400 JSON
error like the other validations; either reuse an existing MAX_ORG_ID_LENGTH
constant or define one near the top of the file (or validation helpers) and use
that constant in the loop to validate trimmed.length before adding to
seen/normalizedOrgIds so overly long IDs never proceed to membership lookup or
paperOrgs updates.
- Around line 1321-1329: 現状、db.update(papers).set(updates).where(eq(papers.id,
paperId)) と paperOrgs の delete/insert（paperOrgs, finalOrgIds, nextVisibility,
hasVisibilityInBody, shouldReplacePaperOrgs）が別々の await
になっており途中失敗で不整合になるため、これらを単一トランザクションにまとめてください。具体的には db.transaction(...)
を使い、トランザクション内で先に db.update(papers).set(updates).where(eq(papers.id, paperId))
を実行し、その後の paperOrgs に対する delete と必要なら insert(finalOrgIds.map(...))
を同じトランザクション（tx.delete/tx.insert）で実行するように変更してください。
In `@apps/web/src/app/papers/`[id]/edit/page.tsx:
- Around line 193-204: The guard in the save flow incorrectly blocks saving when
visibility === "org_only" solely because organizations.length === 0, even if the
paper already has organizations in data.organizations or the PATCH can omit
unchanged orgIds; update the checks in the component handling visibility (use
the variables visibility, organizations, selectedOrgIds and data.organizations)
so that you only setError and return when there are no current org selections
anywhere (i.e., organizations.length === 0 AND (data.organizations is empty or
undefined) AND selectedOrgIds.length === 0); if data.organizations exists
(non-empty) allow the save to proceed and omit orgIds from the PATCH, otherwise
enforce selecting at least one organization and keep the existing error
messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 027f94c5-eb31-4770-93af-2dde9979fa95
📒 Files selected for processing (5)
apps/api/src/routes/__tests__/papers.test.tsapps/api/src/routes/__tests__/users.test.tsapps/api/src/routes/papers.tsapps/web/src/app/__tests__/paper-edit-page.test.tsxapps/web/src/app/papers/[id]/edit/page.tsx
Contributor
Author
|
メンテナ確認済みです。Botコメント（Codecov / CodeRabbit / Gemini / Jules / Greptile / Vercel）を確認し、現時点で追加対応が必要な指摘はありません。必要な追対応が出た場合はこのPRで反映します。 |
…mplete # Conflicts: # apps/web/src/app/__tests__/paper-edit-page.test.tsx # apps/web/src/app/papers/[id]/edit/page.tsx
Merging this PR will not alter performance
Comparing 
Footnotes
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/routes/__tests__/papers.test.ts (1)
556-621: 🧹 Nitpick | 🔵 Trivial
org_only解除時のpaperOrgs削除分岐も固定しておきたいです。今回の追加ケースは「org_only に入る/置き換える」側は押さえていますが、
apps/api/src/routes/papers.tsLine 1264-1265 の「org_only から外れるときにpaperOrgsを削除する」分岐は未検証です。ここが壊れると private/public に戻した後も組織アクセスが残りうるので、1 本追加しておくと安心です。🧪 追加したいテストの最小イメージ
+ it("PATCH /api/papers/:id clears paperOrgs when leaving org_only", async () => { + // initial visibility: org_only + // request: { visibility: "private" } + // expect: mockDb.delete called once, mockDb.insert not called + });Also applies to: 821-856
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/__tests__/papers.test.ts` around lines 556 - 621, Add a test that asserts paperOrgs are removed when changing visibility away from org_only: mock the initial select to return the paper with visibility "org_only" and an existing paperOrgs row (e.g., { orgId: "org-1" }), stub mockDb.delete to return an object with a where() spy (like deleteWhere), then send a PATCH to the papers endpoint with visibility set to "private" (or "public") and no orgIds; assert status 200 and that deleteWhere was called once and mockDb.insert/insertValues was not called. Reference the existing test scaffolding (the PATCH /api/papers/:id tests, mocks mockDb.select, mockDb.delete, deleteWhere, mockDb.insert/insertValues) and the papers.ts branch that removes paperOrgs when leaving "org_only" (the org_only -> non-org_only deletion branch).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/papers.ts`:
- Around line 1230-1270: The code currently sets shouldReplacePaperOrgs = true
whenever orgIdsFromBody !== undefined, causing DELETE→INSERT even if the
submitted orgIds equal the current ones; instead, query the existing org IDs for
the paper (select orgId from paperOrgs where paperId = paperId), compare that
set to finalOrgIds (treat as sets, ignoring order/duplicates) and only set
shouldReplacePaperOrgs = true when they differ. Update the logic around
orgIdsFromBody/finalOrgIds/shouldReplacePaperOrgs so no DB delete/insert
operations are enqueued when the two sets are identical.
- Around line 1236-1245: The file uses the orgMembers symbol in the memberships
query but it is not imported, causing TypeScript errors; add orgMembers to the
existing schema imports at the top of the file (where other table schemas are
imported) so the query using orgMembers, db.select, and inArray/eq compiles and
the memberships query resolves correctly.
In `@apps/web/src/app/papers/`[id]/__tests__/paper-detail-client.test.tsx:
- Line 464: There is a duplicate spy on console.error: remove the redundant
vi.spyOn(console, "error").mockImplementation(() => {}); in the test at the
later location and rely on the existing spy set up in the beforeEach (or vice
versa), ensuring only one spy setup remains (the one in beforeEach or the one
intended for that specific test) so tests don’t double-mock console.error;
update/keep the single vi.spyOn(console, "error") usage and remove the other to
clarify intent.
In `@apps/web/src/app/papers/`[id]/edit/page.tsx:
- Around line 43-50: The code awaits
apiFetch(`/api/papers/${encodeURIComponent(paperId)}`) and then serially awaits
apiFetch("/api/users/me/orgs"), causing an extra RTT; instead fire both requests
in parallel (e.g., start both promises and use Promise.allSettled or
Promise.all) so paper fetch isn't delayed by orgs, keep the existing behavior of
swallowing orgs errors by checking the settled result for the orgs promise and
setting orgsRes to null (and optionally logging a warning) while still using the
paperRes value immediately; locate the promises around paperRes, orgsRes and
apiFetch to implement this change.
---
Outside diff comments:
In `@apps/api/src/routes/__tests__/papers.test.ts`:
- Around line 556-621: Add a test that asserts paperOrgs are removed when
changing visibility away from org_only: mock the initial select to return the
paper with visibility "org_only" and an existing paperOrgs row (e.g., { orgId:
"org-1" }), stub mockDb.delete to return an object with a where() spy (like
deleteWhere), then send a PATCH to the papers endpoint with visibility set to
"private" (or "public") and no orgIds; assert status 200 and that deleteWhere
was called once and mockDb.insert/insertValues was not called. Reference the
existing test scaffolding (the PATCH /api/papers/:id tests, mocks mockDb.select,
mockDb.delete, deleteWhere, mockDb.insert/insertValues) and the papers.ts branch
that removes paperOrgs when leaving "org_only" (the org_only -> non-org_only
deletion branch).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad58dccb-2e08-4183-9138-b3132ee2055e
📒 Files selected for processing (7)
apps/api/src/routes/__tests__/papers.test.tsapps/api/src/routes/__tests__/users.test.tsapps/api/src/routes/papers.tsapps/web/src/app/__tests__/paper-edit-page.test.tsxapps/web/src/app/papers/[id]/__tests__/paper-detail-client.test.tsxapps/web/src/app/papers/[id]/edit/page.tsxapps/web/src/components/papers/edit-form.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/papers/`[id]/__tests__/paper-detail-client.test.tsx:
- Line 550: Remove the redundant call to fetchSpy.mockRestore() in the test
teardown; the existing afterEach block already calls vi.restoreAllMocks(), so
delete the fetchSpy.mockRestore() invocation (referencing fetchSpy.mockRestore
and the afterEach that calls vi.restoreAllMocks()) to avoid duplicate mock
restoration.
- Around line 463-551: The test overcomplicates failure simulation by spying on
window.fetch; instead modify the apiFetch mock used in this test so when
url.includes("/files/file-image/stream") and method === "GET" it directly
returns a rejected promise or throws an Error to simulate the network failure
(affecting the test's apiFetch mock implementation), remove the window.fetch
spy/fetchSpy usage and its restore, and keep assertions that PaperDetailClient
(rendered with paperId="paper-1") logs the error and shows the "画像の読み込みに失敗しました"
message.
In `@apps/web/src/app/papers/`[id]/edit/page.tsx:
- Around line 88-89: 不要既存の組織メタデータを捨てないでください — 現状 setInitialSelectedOrgIds が
data.organizations を id 配列に変換しているため、PaperEditForm に表示用の name/slug 等が渡っていません。修正は
data.organizations 全体を保持してフォームに渡すようにし、selected IDs は別で生成すること（参照: setPaperData,
setInitialSelectedOrgIds, PaperEditForm,
data.organizations）；同様の変更をファイル内の別箇所（該当ブロック 122-129 に相当する処理）にも適用してください。
In `@apps/web/src/components/papers/edit-form.tsx`:
- Around line 136-149: The guard wrongly uses
areSameOrgSelection(selectedOrgIds, initialSelectedOrgIds) directly (which
returns true for [] vs []) allowing an empty selection to pass; change the
conditional to use the previously computed keepingExistingSelection (which
already checks initialSelectedOrgIds.length > 0) in the second check so the
empty-initial case is not treated as preserving selection; update the identical
condition later (the block around methods handling includeOrgIds/visibility) to
use keepingExistingSelection as well and keep using setError, selectedOrgIds,
initialSelectedOrgIds, areSameOrgSelection, and includeOrgIds unchanged
otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ec186660-90d5-4a68-aa5f-7982e4b58299
📒 Files selected for processing (5)
apps/api/src/routes/__tests__/papers.test.tsapps/api/src/routes/papers.tsapps/web/src/app/papers/[id]/__tests__/paper-detail-client.test.tsxapps/web/src/app/papers/[id]/edit/page.tsxapps/web/src/components/papers/edit-form.tsx
is0692vs
commented
Mar 22, 2026
This was referenced Mar 28, 2026
Closed
Closed
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.
概要
org_only 公開範囲の論文を、作成から編集まで一貫して正しく扱えるようにしました。
変更点
テスト
apps/api/src/routes/__tests__/papers.test.tsapps/web/src/app/__tests__/paper-edit-page.test.tsxGreptile Summary
このPRは
org_only公開範囲の論文について、作成から編集まで一貫して正しく扱えるようにするもので、API・フロントエンド・テストにわたる包括的な対応です。以前は編集画面でorg_onlyへの変更が完全にブロックされていましたが、今回の変更により組織選択 UI とそれに対応するバックエンド検証（メンバーシップチェック・paperOrgsの置換）が実装されました。主な変更点:
GET /api/papers/:id:paperOrgsJOIN を追加し、関連組織を常に返すよう変更PATCH /api/papers/:id:orgIdsの受付・検証・メンバーシップチェック・paperOrgsの delete+insert を実装。org_only以外への変更時はpaperOrgsを削除org_only選択時に組織チェックボックス UI を表示org_only編集ケース（成功・バリデーション失敗・メンバー外拒否）を網羅するテストを追加指摘事項:
loadingOrgs状態はloadingと同じfinallyブロックでfalseにセットされるため、フォームが表示される時点では常にfalseであり、submit ガードとフィールドセット内のローディング表示はデッドコードになっているorg_only論文の保存時に、組織選択が変更されていなくても毎回paperOrgsの全削除・再挿入が実行される（フロントエンドが常にorgIdsを送信するため）GET /api/papers/:idで visibility に関係なく全論文で組織情報の JOIN が実行されるようになり、ホットパスにパフォーマンスオーバーヘッドが生じる可能性があるif (orgIdsFromBody && ...)は JavaScript の空配列が truthy であることを前提とした条件式であり、orgIdsFromBody !== undefinedとした方が意図が明確Confidence Score: 3/5
loadingOrgsのデッドコード、保存ごとの不要な paperOrgs 全置換、GET ホットパスへの JOIN 追加という品質・パフォーマンス懸念が複数存在するため、満点ではない。apps/api/src/routes/papers.ts（GET の JOIN オーバーヘッドと PATCH の paperOrgs 置換ロジック）とapps/web/src/app/papers/[id]/edit/page.tsx（loadingOrgs デッドコード・orgIds 常時送信）に注意が必要Important Files Changed
loadingOrgsが到達不能なデッドコード化しており、また毎回の保存で paperOrgs を不必要に全置換する非効率な実装が含まれている。Sequence Diagram
sequenceDiagram participant Browser as ブラウザ (編集ページ) participant API as API サーバー participant DB as データベース Browser->>API: GET /api/papers/:id API->>DB: SELECT paper API->>DB: batch([paperFiles, paperAuthors, paperOrgs+orgs]) DB-->>API: paper情報 + organizations[] API-->>Browser: { paper, authors, organizations } Browser->>API: GET /api/users/me/orgs API-->>Browser: { organizations: UserOrganization[] } Note over Browser: ユーザーが visibility=org_only を選択<br/>チェックボックスで対象組織を選択 Browser->>API: PATCH /api/papers/:id<br/>{ visibility: "org_only", orgIds: ["org-1"] } API->>DB: SELECT paper (visibility確認) API->>DB: SELECT paperAuthors (著者確認) API->>DB: SELECT orgMembers WHERE orgId IN orgIds (メンバーシップ検証) alt メンバーでない場合 API-->>Browser: 403 Forbidden else 正常 API->>DB: UPDATE papers SET visibility, updatedAt API->>DB: DELETE paperOrgs WHERE paperId API->>DB: INSERT paperOrgs (paperId, orgId) × n件 API-->>Browser: { ok: true } endComments Outside Diff (2)
apps/web/src/app/papers/[id]/edit/page.tsx, line 208-213 (link)org_only保存時に毎回paperOrgsを全削除・再挿入orgIdsはフォーム保存のたびに常に送信されます（visibility === "org_only"の場合）。API 側ではこれを受け取ると、組織の選択が変更されていなくても毎回paperOrgsを DELETE → INSERT で完全に置き換えます（papers.ts1307〜1311行目）。対象組織を変更しないまま他のフィールド（タイトルなど）のみを更新した場合でも、不必要な DB 書き込みが発生します。フロントエンド側で初期値と現在値を比較して変更があった場合のみ
orgIdsを送信するか、API 側で現在のpaperOrgsと比較してから更新するアプローチを検討してください。または、サブミット時に変更フラグを保持する実装に変更することを推奨します。
Prompt To Fix With AI
apps/api/src/routes/papers.ts, line 541-578 (link)GET /api/papers/:idエンドポイントでは、visibilityに関わらず全論文でpaperOrgsとorgsの JOIN を常に実行するようになりました。publicやprivateの論文ではorganizationsは常に空配列になるにもかかわらず、毎回余分なクエリが走ります。このエンドポイントは論文ビューページからも呼び出される可能性が高く、ホットパスになりえます。
visibility === 'org_only'の場合のみ組織を取得するか、編集ページ専用のエンドポイント（例:GET /api/papers/:id/edit）を用意することで、パフォーマンスへの影響を最小化できます。Prompt To Fix With AI
Prompt To Fix All With AI
Last reviewed commit: "feat: support full o..."