Skip to content

Attachments & Quotes に対応 - #3097

Open
HokubuSubway wants to merge 36 commits into
masterfrom
feat/issue2974
Open

Attachments & Quotes に対応#3097
HokubuSubway wants to merge 36 commits into
masterfrom
feat/issue2974

Conversation

@HokubuSubway

@HokubuSubway HokubuSubway commented Jul 1, 2026

Copy link
Copy Markdown

Attachments & Quotes に対応

Summary by CodeRabbit

  • New Features
    • Extended GET /channels/{channelId}/messages with optional include-attachments and include-quotes query parameters.
    • Channel message responses now return a richer payload that includes attachments and quotes.
    • Timeline and search message results were updated to expose the richer message details.
  • Bug Fixes
    • Message results now return enriched message details consistently across search and fetch flows.
  • Documentation
    • Updated OpenAPI/Swagger definitions (including the new MessageNew schema) to match the expanded response format.

@HokubuSubway
HokubuSubway requested a review from a team as a code owner July 1, 2026 07:18
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds optional include-attachments/include-quotes query parameters and a MessageNew schema to the messages API, introduces a MessageNew persistence model and Detailed message interface with a messageNew wrapper, updates message retrieval and timeline JSON to include attachments and quotes, adjusts search result typing, and changes Wire's Server field injection.

Changes

Attachments and quotes inclusion feature

Layer / File(s) Summary
API contract and model additions
docs/v3-api.yaml, docs/swagger.yaml, model/messages.go
Adds includeAttachments/includeQuotes query parameters, parameter definitions, and MessageNew schemas to the OpenAPI specs; adds a MessageNew GORM struct with Attachments []*FileMeta and Quotes []*Message fields.
Detailed interface, manager retrieval, and wrapper implementation
service/message/model.go, service/message/manager.go, service/message/manager_impl.go, service/message/model_impl.go
Adds Detailed interface (Message + GetAttachments/GetQuotes + json.Marshaler); Manager.GetIn now returns []Detailed, parsing text to resolve attachment FileMeta and quoted messages; adds messageNew wrapper with getters, stamp caching, and MarshalJSON.
Timeline model and JSON output
service/message/timeline_impl.go
Switches timeline records and wrapped timeline messages to MessageNew, and adds attachments and quotes to the preloaded JSON payload.
Search result mapping
service/search/es_result.go
Changes messagesMap callback type from message.Message to message.Detailed, still mapping by GetID() for ordering.

Server wiring field selection update

Layer / File(s) Summary
Wire field selection
cmd/serve_wire.go
Changes Server struct injection from wildcard "*" to explicit fields "L", "SS", "Router", "Hub", "Repo".

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Search as esEngine
  participant Manager as message.Manager
  participant Parse as utils/message
  participant Repo as repository

  Search->>Manager: GetIn(ctx, ids)
  Manager->>Repo: GetMessages(ids)
  Repo-->>Manager: base messages
  Manager->>Parse: Parse(mm.Text)
  Parse-->>Manager: attachment IDs, quote IDs
  Manager->>Repo: GetFileMeta(attachment IDs)
  Repo-->>Manager: FileMeta
  Manager->>Repo: GetMessages(quote IDs)
  Repo-->>Manager: quoted messages
  Manager-->>Search: []Detailed
Loading

Suggested reviewers: ramdos0207

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly reflects the main change: adding support for attachments and quotes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/issue2974

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Nitpick comments (2)
service/message/model.go (1)

25-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reduce duplication by embedding Message.

MessageNew repeats every method already declared in Message. Embedding Message and only adding the two new getters keeps both interfaces in sync and removes the duplication.

♻️ Proposed refactor
 type MessageNew interface {
-	GetID() uuid.UUID
-	GetUserID() uuid.UUID
-	GetChannelID() uuid.UUID
-	GetText() string
-	GetCreatedAt() time.Time
-	GetUpdatedAt() time.Time
-	GetStamps() []model.MessageStamp
-	GetPin() *model.Pin
+	Message
 	GetAttachments() []*model.FileMeta
 	GetQuotes() []*model.Message
-
-	json.Marshaler
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@service/message/model.go` around lines 25 - 38, Reduce duplication in
MessageNew by embedding Message instead of repeating its getters. Update the
MessageNew interface in Message to include Message and keep only the additional
methods needed by the new type, while preserving json.Marshaler so both
interfaces stay in sync automatically.
service/search/es_result.go (1)

60-60: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift

Unconditional true, true triggers per-message N+1 fetches on every search request.

GetIn loops over each returned message to fetch attachments (one GetFileMeta call per attachment) and separately fetches quotes, per the upstream implementation. Hardcoding both flags to true here means every search response now pays this cost regardless of whether the caller/UI needs attachments or quotes, which could noticeably slow down search under load with many hits.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@service/search/es_result.go` at line 60, The search path in e.searchMessages
is always calling mm.GetIn with both attachment and quote expansion enabled,
which forces expensive per-message lookups on every request. Update the GetIn
call to pass flags based on the actual search response needs in es_result.go,
and only enable attachments/quotes when the caller explicitly requires them. Use
the e.mm.GetIn invocation and the surrounding search result assembly to keep the
behavior unchanged for clients that don’t need those fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@model/messages.go`:
- Around line 26-42: MessageNew is duplicating the full persistence schema from
Message, including GORM tags and constraint names, which can drift and collide
during migration. Refactor MessageNew in messages.go to embed Message instead of
redefining all base fields, and keep only the extra association fields unique to
MessageNew. Update any construction sites that currently populate MessageNew
field-by-field to initialize the embedded Message struct directly.

In `@service/message/manager_impl.go`:
- Around line 77-83: The attachment-loading loop in manager_impl.go currently
stops on the first GetFileMeta error and silently drops the rest of pRa. Update
the loop in the attachment fetch logic to log the GetFileMeta failure with
context, then continue iterating so later valid attachments are still appended
to aR instead of being skipped.
- Around line 73-88: The `ManagerImpl` message-mapping logic is doing
per-message DB lookups in `utils.Map`, causing an N+1 pattern for attachments
and quotes. Refactor the `messageParse.Parse` / `GetFileMeta` / `GetMessages`
flow to first collect all attachment IDs and citation IDs across the batch,
fetch them once with batched repository calls, and then map the returned
`FileMeta` and message quote data back onto each `MessageNew`.
- Around line 73-88: The attachment and quote lookup work in MessageNewList is
running unconditionally, so the ia and iq flags are not respected. Update the
logic in manager_impl.go within MessageNewList to gate the messageParse.Parse,
GetFileMeta loop, and GetMessages quote fetch behind the corresponding ia/iq
checks, using the existing MessagesQuery flags already passed into
repository.MessagesQuery. Keep the current behavior only when the flag is
enabled, and skip parsing/DB lookups entirely when ia or iq is false.
- Around line 84-88: The quote lookup in manager_impl.go currently returns a nil
MessageNew from the mapping path, which can later panic and hides the failure.
Update the GetIn flow in the method that uses parseResult.Citation and
m.R.GetMessages so the error is propagated instead of returning nil, and make
GetIn surface that error to callers rather than always returning ret, nil. Use
the existing MessageNew-related path and utils.Map/GetIn logic to locate the
fix.

In `@service/message/model_impl.go`:
- Around line 237-263: `messageNew.MarshalJSON` is still serializing the base
message shape and omits attachments/quotes entirely. Update the `obj` payload
and the `v` construction in `messageNew.MarshalJSON` to include fields populated
from `GetAttachments()` and `GetQuotes()` alongside `GetStamps()`, using the
existing `messageNew`/`MarshalJSON` symbols so the JSON response actually
carries the extra data fetched by `GetIn`.

In `@service/search/es_result.go`:
- Line 60: The search result parsing path is creating a new background context
instead of using the request-scoped one. Update parseResultFromResponse to
accept context.Context as its first parameter, pass that context through from
its callers, and use it when calling e.mm.GetIn so cancellation, deadlines, and
tracing flow through the message/attachment/quote fetches. Ensure any other I/O
helpers in this path continue forwarding the same context rather than
fabricating a new one.

---

Nitpick comments:
In `@service/message/model.go`:
- Around line 25-38: Reduce duplication in MessageNew by embedding Message
instead of repeating its getters. Update the MessageNew interface in Message to
include Message and keep only the additional methods needed by the new type,
while preserving json.Marshaler so both interfaces stay in sync automatically.

In `@service/search/es_result.go`:
- Line 60: The search path in e.searchMessages is always calling mm.GetIn with
both attachment and quote expansion enabled, which forces expensive per-message
lookups on every request. Update the GetIn call to pass flags based on the
actual search response needs in es_result.go, and only enable attachments/quotes
when the caller explicitly requires them. Use the e.mm.GetIn invocation and the
surrounding search result assembly to keep the behavior unchanged for clients
that don’t need those fields.
🪄 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: CHILL

Plan: Pro

Run ID: cdeb03cd-d854-410b-9de9-f728339e9ec1

📥 Commits

Reviewing files that changed from the base of the PR and between 90392d3 and fb54348.

📒 Files selected for processing (8)
  • docs/v3-api.yaml
  • model/messages.go
  • repository/message.go
  • service/message/manager.go
  • service/message/manager_impl.go
  • service/message/model.go
  • service/message/model_impl.go
  • service/search/es_result.go

Comment thread model/messages.go Outdated
Comment thread service/message/manager_impl.go Outdated
Comment thread service/message/manager_impl.go Outdated
Comment on lines +77 to +83
for i := 0; i < len(pRa); i++ {
attachment, err := m.R.GetFileMeta(ctx, pRa[i])
if err != nil {
break
}
aR = append(aR, attachment)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Attachment fetch errors are silently swallowed.

On the first GetFileMeta error, the loop breaks and any remaining attachments in pRa are silently dropped — no logging, no error surfaced to caller. Consider logging the error and using continue instead of break so a single missing file doesn't drop all subsequent valid attachments.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@service/message/manager_impl.go` around lines 77 - 83, The attachment-loading
loop in manager_impl.go currently stops on the first GetFileMeta error and
silently drops the rest of pRa. Update the loop in the attachment fetch logic to
log the GetFileMeta failure with context, then continue iterating so later valid
attachments are still appended to aR instead of being skipped.

Comment thread service/message/manager_impl.go Outdated
Comment thread service/message/model_impl.go Outdated
Comment thread service/search/es_result.go Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
service/search/es_result.go (1)

47-47: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Still fabricating context.Background() instead of propagating caller's context.

parseResultFromResponse doesn't accept context.Context and calls e.mm.GetIn (which performs message, attachment, and quote I/O) with context.Background() at Line 60, dropping cancellation/deadline/tracing for the request.

As per path instructions, "Functions performing I/O or network requests must accept context.Context as their first parameter" and "The received context must be passed down to all subsequent calls."

Also applies to: 60-60

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@service/search/es_result.go` at line 47, The result parsing path is dropping
caller context by using a fabricated background context for I/O inside
parseResultFromResponse. Update esEngine.parseResultFromResponse to accept
context.Context as the first parameter, then pass that context through to
e.mm.GetIn (and any related message/attachment/quote lookups) instead of
context.Background(), and ensure the caller in the search flow forwards its
existing context into this method.

Source: Path instructions

🧹 Nitpick comments (1)
service/message/model.go (1)

25-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reduce duplication by embedding Message.

MessageWithAttachmentsAndQuotes repeats every getter from Message verbatim. Embedding Message and adding only the new methods avoids drift if Message changes.

♻️ Proposed refactor
 type MessageWithAttachmentsAndQuotes interface {
-	GetID() uuid.UUID
-	GetUserID() uuid.UUID
-	GetChannelID() uuid.UUID
-	GetText() string
-	GetCreatedAt() time.Time
-	GetUpdatedAt() time.Time
-	GetStamps() []model.MessageStamp
-	GetPin() *model.Pin
+	Message
 	GetAttachments() []*model.FileMeta
 	GetQuotes() []*model.Message
-
-	json.Marshaler
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@service/message/model.go` around lines 25 - 38, Refactor
MessageWithAttachmentsAndQuotes to embed Message instead of repeating all of its
getter methods, and keep only the additional attachment/quote accessors plus
json.Marshaler. Update any implementations or references that rely on the
duplicated Message methods so they still satisfy the interface via the embedded
Message, preventing drift when Message changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@service/search/es_result.go`:
- Line 47: The result parsing path is dropping caller context by using a
fabricated background context for I/O inside parseResultFromResponse. Update
esEngine.parseResultFromResponse to accept context.Context as the first
parameter, then pass that context through to e.mm.GetIn (and any related
message/attachment/quote lookups) instead of context.Background(), and ensure
the caller in the search flow forwards its existing context into this method.

---

Nitpick comments:
In `@service/message/model.go`:
- Around line 25-38: Refactor MessageWithAttachmentsAndQuotes to embed Message
instead of repeating all of its getter methods, and keep only the additional
attachment/quote accessors plus json.Marshaler. Update any implementations or
references that rely on the duplicated Message methods so they still satisfy the
interface via the embedded Message, preventing drift when Message changes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d434a2f5-f198-4e4f-9488-e5642d7d656c

📥 Commits

Reviewing files that changed from the base of the PR and between fb54348 and a3f94bc.

📒 Files selected for processing (4)
  • service/message/manager.go
  • service/message/manager_impl.go
  • service/message/model.go
  • service/search/es_result.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • service/message/manager.go
  • service/message/manager_impl.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
service/message/manager_impl.go (1)

68-88: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win

includeAttachments/includeQuotes are not wired up

router/v3/utils.go:107-117 doesn’t bind these query params, so router/v3/messages.go drops them before reaching MessageManager. service/message/manager_impl.go:68-88 then always parses text and fetches attachments/quotes, so the documented flags can’t skip the extra DB work or change the response shape.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@service/message/manager_impl.go` around lines 68 - 88, The includeAttachments
and includeQuotes flags are not being propagated into MessageManager, so GetIn
always does attachment and quote lookups. Update the router query binding and
the message retrieval flow so these params are carried through from the request
into manager.GetIn, then use them inside manager_impl.go’s GetIn method to
conditionally skip Parse/GetFileMeta/GetMessages work and omit those fields from
Detailed when disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cmd/serve.go`:
- Around line 201-207: The Wire helper providers provideCancelFunc and
provideShutdownChan are currently unused, while the startup path still creates
the cancel context and shutdown channel inline. Either remove these helpers from
cmd/serve.go if they are not intended to be part of dependency injection, or add
them to the Wire setup in serve_wire/wire_gen so Serve startup actually uses
them instead of the inline setup. Make the change consistently around the serve
startup wiring and any references to context.WithCancel and make(chan struct{}).

---

Outside diff comments:
In `@service/message/manager_impl.go`:
- Around line 68-88: The includeAttachments and includeQuotes flags are not
being propagated into MessageManager, so GetIn always does attachment and quote
lookups. Update the router query binding and the message retrieval flow so these
params are carried through from the request into manager.GetIn, then use them
inside manager_impl.go’s GetIn method to conditionally skip
Parse/GetFileMeta/GetMessages work and omit those fields from Detailed when
disabled.
🪄 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: CHILL

Plan: Pro

Run ID: 64387577-0f8e-4574-a68b-e2e0d24f0ec6

📥 Commits

Reviewing files that changed from the base of the PR and between a3f94bc and 89d0a07.

📒 Files selected for processing (6)
  • cmd/serve.go
  • cmd/serve_wire.go
  • service/message/manager.go
  • service/message/manager_impl.go
  • service/message/model.go
  • service/search/es_result.go

Comment thread cmd/serve.go Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
docs/v3-api.yaml (1)

4782-4848: 📐 Maintainability & Code Quality | 🔵 Trivial

Consider allOf composition to reduce schema duplication.

MessageNew duplicates every property of Message verbatim plus attachments/quotes. Using allOf: [$ref Message, extra properties] would avoid the two schemas drifting out of sync on future edits.

♻️ Example refactor using allOf
-    MessageNew:
-      title: MessageNew
-      type: object
-      description: メッセージ 詳細
-      properties:
-        id: ...
-        userId: ...
-        ... (full duplication)
-      required:
-        - id
-        - userId
-        ...
+    MessageNew:
+      title: MessageNew
+      description: メッセージ 詳細
+      allOf:
+        - $ref: "`#/components/schemas/Message`"
+        - type: object
+          properties:
+            attachments:
+              type: array
+              description: 添付ファイルの配列
+              items:
+                $ref: "`#/components/schemas/FileInfo`"
+            quotes:
+              type: array
+              description: 引用メッセージの配列
+              items:
+                $ref: "`#/components/schemas/Message`"
+          required:
+            - attachments
+            - quotes
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/v3-api.yaml` around lines 4782 - 4848, MessageNew currently duplicates
the full Message schema, which can drift out of sync over time. Refactor the
MessageNew schema in the v3 API definition to compose from Message using allOf,
then add only the extra MessageNew-specific properties such as attachments and
quotes. Keep the existing MessageNew identifier and ensure any required fields
remain correct after composition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@docs/v3-api.yaml`:
- Around line 4782-4848: MessageNew currently duplicates the full Message
schema, which can drift out of sync over time. Refactor the MessageNew schema in
the v3 API definition to compose from Message using allOf, then add only the
extra MessageNew-specific properties such as attachments and quotes. Keep the
existing MessageNew identifier and ensure any required fields remain correct
after composition.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: eacc990b-3e7f-4ead-bb55-07fb83faaf3d

📥 Commits

Reviewing files that changed from the base of the PR and between 89d0a07 and 5512ece.

📒 Files selected for processing (2)
  • docs/swagger.yaml
  • docs/v3-api.yaml

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
service/message/timeline_impl.go (1)

93-110: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Don’t serialize raw persistence messages as quote payloads.

Quotes is emitted as []*model.Message, unlike the top-level DTO mapping above. Since model.Message lacks the lower-camel/content JSON mapping and may include preloaded associations, quoted messages can leak internal shape or extra nested data. Map quotes through the same API DTO/wrapper before assigning them.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@service/message/timeline_impl.go` around lines 93 - 110, The Quotes field in
timeline_impl.go is currently assigned raw []*model.Message values, which can
leak persistence shape and nested associations. Update the objectWithPreload
mapping in the preloaded branch to convert each quote through the same API
DTO/wrapper used for top-level messages, so Quotes contains serialized message
payloads instead of model.Message instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@service/message/manager_impl.go`:
- Around line 130-133: The MessageNew building path in manager_impl.go is always
doing full enrichment even when query.DisablePreload is true, but
timelineMessage.MarshalJSON will skip using attachments/quotes in that case.
Update the loop around buildMessageNew so it takes preload/include flags or
branches early to construct a lightweight MessageNew when preloading is
disabled, avoiding text parsing and attachment/quote DB lookups for those
records.
- Around line 73-80: Apply the caller’s visibility constraints before resolving
any referenced files or messages in the message manager flow. Update the helper
in manager_impl.go so the file metadata lookup via GetFileMeta and the quote
lookup via GetMessages both validate access using the caller’s timeline/scope,
not just the raw IDs from parseResult.Citation. Thread the caller context or
visibility filter through this path, or explicitly authorize each resolved
reference before appending attachments or quotes.

---

Outside diff comments:
In `@service/message/timeline_impl.go`:
- Around line 93-110: The Quotes field in timeline_impl.go is currently assigned
raw []*model.Message values, which can leak persistence shape and nested
associations. Update the objectWithPreload mapping in the preloaded branch to
convert each quote through the same API DTO/wrapper used for top-level messages,
so Quotes contains serialized message payloads instead of model.Message
instances.
🪄 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: CHILL

Plan: Pro

Run ID: 6638078f-1aa1-4bf0-86de-1852c9caad01

📥 Commits

Reviewing files that changed from the base of the PR and between 5512ece and fb9e53a.

📒 Files selected for processing (3)
  • service/message/manager_impl.go
  • service/message/model_impl.go
  • service/message/timeline_impl.go

Comment thread service/message/manager_impl.go Outdated
Comment thread service/message/manager_impl.go Outdated
Comment thread service/message/model.go Outdated
Comment thread docs/v3-api.yaml
- pinned
- stamps
- threadId
DetailedMessage:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenAPI では allOf を利用するとプロパティのマージができるので、Message を利用できそう

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gogen が場合によって allOf のマージを完全に処理できない場合があるようなので,見送ります

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

今回はその場合に当てはまるの?

Comment thread model/messages.go
Comment thread router/v3/utils.go Outdated
Comment thread router/v3/utils.go Outdated
Comment thread service/message/manager_impl.go Outdated
Comment thread service/message/manager.go Outdated
Comment thread service/message/manager_impl.go Outdated
Comment thread service/message/model_impl.go Outdated
sync.RWMutex
}

type DetailedMessage struct {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここも埋め込みを使うと良いかも

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここは埋め込みによるメリットがあまりないので,そのまま使用します

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

埋め込みを使ったらその下のメソッドの重複も避けられない? (そんなことない?)

@cp-20 cp-20 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

またいくつかコメントしました 基本的にコードは後から読まれることを意識して書いて欲しくて、そのためにいくつか意識すべき原則的なことを書いておきます

  • 変数名とか関数名とか構造体名とかは分かりやすいものにする
    • (慣習としてそうなっていない限り) 1文字の変数などは避ける
    • 今回の例でいえば a という名前の変数を後から見た時に、何なのかが分かりますか?
  • コードの (本質的な) 重複は避ける
    • 後からコードを編集したときに実装漏れなどが生じる可能性があるため
    • 今回の例でいえば Message に新しいプロパティが付け加えられたときに、DetailedMessage と被っている部分を忘れずに編集することができるでしょうか? (できないことはないんだけど、忘れる可能性がある)

Comment thread model/messages.go
Comment thread service/message/build_detailed_message_test.go
Comment thread service/message/build_detailed_message_test.go
Comment thread service/message/model.go Outdated
Comment thread service/message/manager_impl.go Outdated
Comment thread docs/swagger.yaml

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

こっちは v2 の API っぽいから、変えなくても良さそう

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

これも対応して欲しいな

@cp-20 cp-20 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

そろそろコードも整ってきたので、動作的な部分にも踏み込んでコメントしてみました 特に権限チェックの不備は脆弱性になり得るので確認してみて欲しいです

Comment thread router/v3/utils.go
Comment on lines +114 to +115
IncludeAttachments bool `query:"include-attachments"`
IncludeQuotes bool `query:"include-quotes"`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MessageQuery にはちゃんと値が入るけど、これが TimelineQuery にコピーされない (手元で動かしてみればクエリパラメータによって動作が変わらないことが分かるはず)

Comment thread service/message/manager_impl.go Outdated
parseResult := messageParser.Parse(mm.Text)
attachmentsResult := []*model.FileMeta{}
for _, fid := range parseResult.Attachments {
attachment, err := m.R.GetFileMeta(ctx, fid)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ファイルを取得する権限があるかをチェックしていない (e.g. DMに貼られた画像)

if includeQuotes {
citationResult = []*model.QuotedMessage{}
var err error
quotes, _, err := m.R.GetMessages(ctx, repository.MessagesQuery{IDIn: optional.From((parseResult.Citation))})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

引用を取得する権限があるかをチェックしていない (e.g. DMのメッセージ)
& 引用の順番がメッセージの登場順ではなくなる (GetMessages は内部で created_at の昇順/降順でソートする)

Comment thread service/message/timeline_impl.go Outdated
Pinned bool `json:"pinned"`
Stamps []model.MessageStamp `json:"stamps"`
ThreadID optional.Of[uuid.UUID] `json:"threadId"` // TODO
Attachments []*model.FileMeta `json:"attachments"`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FileMeta は DB 用の構造体なので、そのまま JSON にするとキー名がおかしくなるし、外部に露出させる用途ではない 実際に自分で API を叩いてみると OpenAPI スキーマとは違うスキーマ (キー名) で帰ってくることがわかるはず

この場合には FileInfo 構造体などを使うのが良さそう

attachmentsResult = []*model.FileMeta{}
for _, fid := range parseResult.Attachments {
attachment, err := m.R.GetFileMeta(ctx, fid)
if err != nil {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

エラーの場合にどうするべきかの挙動は難しいけど、いったん一つでもエラーが起きたら全体をエラーにするという設計で組んでみて欲しい (今は握りつぶしちゃってる)

Comment thread service/message/model.go Outdated
type DetailedMessage interface {
Message
GetAttachments() []*model.FileMeta
GetQuotes() []model.QuotedMessage

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[]*model.QuotedMessage ではない理由は?

Comment thread docs/swagger.yaml

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

これも対応して欲しいな

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

レイアウトの決定に必要な情報を一回のレスポンスにまとめて返せるようにする

3 participants