Skip to content

Conversation

@mizunofukusayou
Copy link
Contributor

@mizunofukusayou mizunofukusayou commented Aug 16, 2025

close #1223

users/me/dm-channel-listにアクセスすると、自分が参加しているDMチャンネルから、メッセージの更新時刻を基準に新しい順に並べ替えた20件を取得します。

@mizunofukusayou mizunofukusayou requested a review from Pugma August 16, 2025 10:06
}

return dmChannelMapping, repo.db.
Table("dm_channel_mappings").
Copy link
Member

Choose a reason for hiding this comment

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

index 効いてるかだけチェックしてほしいです (問題なければ ok)

Copy link
Contributor

@Pugma Pugma left a comment

Choose a reason for hiding this comment

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

実装ありがとうございます!
まずはこちらのパスの変更の部分について対応をお願いします…!

またこの実装に合わせて、リポジトリ内の /docs/v3-api.yaml への追記もお願いします 🙏

Copy link
Contributor

@Pugma Pugma left a comment

Choose a reason for hiding this comment

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

対応ありがとうございます!
もう少し変更をお願いしたい箇所があるので、こちらもやっていただけるとありがたいです…!

@mizunofukusayou mizunofukusayou requested a review from Pugma August 19, 2025 16:21
@Pugma Pugma requested a review from Copilot August 20, 2025 09:30
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a new API endpoint to retrieve a user's DM channel history sorted by message update time. The endpoint /users/me/dm-channel-list returns up to 20 DM channels ordered by latest message timestamp.

Key changes:

  • Added new API endpoint for fetching DM channel list with latest message ordering
  • Implemented repository method using SQL UNION query to fetch channels by message recency
  • Added comprehensive test coverage for the new endpoint

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
router/v3/router.go Registers the new /users/me/dm-channel-list endpoint
router/v3/channels.go Implements the GetDMChannelList handler with business logic
router/v3/channels_test.go Adds test cases for the new endpoint including auth and ordering validation
repository/channel.go Defines the interface for GetDirectMessageChannelList method
repository/gorm/channel.go Implements the database query using UNION to fetch DM channels by message recency
repository/mock_repository/mock_channel.go Adds mock implementation for testing
docs/v3-api.yaml Documents the new API endpoint specification

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

@Pugma Pugma left a comment

Choose a reason for hiding this comment

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

すごく良くなってきていると思います!
もう少しだけ改善をしてもらえるとよりよいコードになると思うので、もう少しだけお願いします 🙏

Comment on lines 251 to 261
Raw(`(SELECT dm.*, clm.date_time
FROM dm_channel_mappings AS dm
RIGHT JOIN channel_latest_messages AS clm ON dm.channel_id = clm.channel_id
WHERE dm.user1 = ?)
UNION
(SELECT dm.*, clm.date_time
FROM dm_channel_mappings AS dm
RIGHT JOIN channel_latest_messages AS clm ON dm.channel_id = clm.channel_id
WHERE dm.user2 = ?)
ORDER BY date_time DESC
LIMIT 20`, userID, userID).
Copy link
Contributor

Choose a reason for hiding this comment

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

このクエリは
WHERE dm.user1 = ?WHERE dm.user2 = ?
以外で同じ操作を行ったものを UNION で結合していますが、これは
WHERE dm.user1 = ? OR dm.user2 = ?
とまとめることで gorm のメソッドを利用した状態で高効率な検索ができると思います
直前にある GetDirectMessageChannelMapping も参照してみてください 👍

また、 copilot からのレビューも入っていますがこの部分は LEFT でも RIGHT でもない JOIN でいいと思います。{user1user2 のいずれかが UserID と一致する (⇒ user1 user2 いずれも値が空でない) ∧ メッセージが投稿されたことがある} DM チャンネルを取得しようとしているからです
ここは、 INNER JOIN と OUTER JOIN の違いについて調べてみるといいんじゃないかな

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ORの検索を使用すると、インデックスが効かず、フルスキャンになってしまいそうだったので、UNIONで連結させることを考えたのですが、ORでも大丈夫そうですか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JOINについて調べてきました
確かにINNER JOINの方が適してました
ありがとうございます!

description: DMチャンネルが更新された順に入った配列
items:
$ref: "#/components/schemas/DMChannel"
operationId: getUserDMChannelList
Copy link
Member

Choose a reason for hiding this comment

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

20件ってするんじゃなくて limit を option で指定可能な形にできますか?

@mizunofukusayou mizunofukusayou requested a review from a team as a code owner August 30, 2025 11:34
@mizunofukusayou mizunofukusayou requested a review from Pugma August 30, 2025 11:45
@Pugma Pugma requested a review from Copilot September 26, 2025 14:24
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +423 to +434
func (h *Handlers) GetDMChannelList(c echo.Context) error {
myID := getRequestUserID(c)
limitStr := c.QueryParam("limit")
if limitStr == "" {
limitStr = "20"
}
limit, err := strconv.Atoi(limitStr)
if err != nil {
return herror.BadRequest("invalid limit")
}
if limit <= 0 {
limit = 20
Copy link

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

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

The magic number 20 appears twice in this function (lines 427 and 434). Consider extracting this default limit value into a named constant to improve maintainability and avoid duplication.

Suggested change
func (h *Handlers) GetDMChannelList(c echo.Context) error {
myID := getRequestUserID(c)
limitStr := c.QueryParam("limit")
if limitStr == "" {
limitStr = "20"
}
limit, err := strconv.Atoi(limitStr)
if err != nil {
return herror.BadRequest("invalid limit")
}
if limit <= 0 {
limit = 20
const defaultDMChannelListLimit = 20
func (h *Handlers) GetDMChannelList(c echo.Context) error {
myID := getRequestUserID(c)
limitStr := c.QueryParam("limit")
if limitStr == "" {
limitStr = strconv.Itoa(defaultDMChannelListLimit)
}
limit, err := strconv.Atoi(limitStr)
if err != nil {
return herror.BadRequest("invalid limit")
}
if limit <= 0 {
limit = defaultDMChannelListLimit

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@Pugma Pugma left a comment

Choose a reason for hiding this comment

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

コードの可読性と信頼性を高めるために、こちらの修正をお願いします…!
おそらく、現在の状態だと query param で巨大な値が与えられたときに弾くことができないと思います

func (h *Handlers) GetDMChannelList(c echo.Context) error {
myID := getRequestUserID(c)
limitStr := c.QueryParam("limit")
if limitStr == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

copilot の review でもコメントがあるように、ここで query param が追加されたので

traQ/router/v3/channels.go

Lines 249 to 278 in 4a8e33d

type channelEventsQuery struct {
Limit int `query:"limit"`
Offset int `query:"offset"`
Since optional.Of[time.Time] `query:"since"`
Until optional.Of[time.Time] `query:"until"`
Inclusive bool `query:"inclusive"`
Order string `query:"order"`
}
func (q *channelEventsQuery) Validate() error {
if q.Limit == 0 {
q.Limit = 20
}
return vd.ValidateStruct(q,
vd.Field(&q.Limit, vd.Min(1), vd.Max(200)),
vd.Field(&q.Offset, vd.Min(0)),
)
}
func (q *channelEventsQuery) convert(cid uuid.UUID) repository.ChannelEventsQuery {
return repository.ChannelEventsQuery{
Since: q.Since,
Until: q.Until,
Inclusive: q.Inclusive,
Limit: q.Limit,
Offset: q.Offset,
Asc: strings.ToLower(q.Order) == "asc",
Channel: cid,
}
}
を参考にして query param の validation を別関数に切り出せるとさらに処理がすっきりするかと思います!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DMの履歴みたいなのが欲しい

4 participants