patch/threadの実装 - #3127
Conversation
📝 WalkthroughWalkthroughAdds thread channel editing through a new manager operation and authenticated ChangesThread channel editing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant EditThread
participant ChannelManager
participant Repository
participant ChannelTree
Client->>EditThread: PATCH thread with name or archived
EditThread->>ChannelManager: UpdateThread(ctx, id, args)
ChannelManager->>Repository: validate and persist thread update
ChannelManager->>ChannelTree: update in-memory thread
ChannelManager-->>EditThread: return result
EditThread-->>Client: 204 or mapped error response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@router/v3/channels.go`:
- Around line 176-184: Add a case for channel.ErrInvalidChannelType in the error
switch within the thread update handler, returning herror.BadRequest with an
appropriate message; keep unexpected errors mapped to
herror.InternalServerError.
🪄 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: 1ff24015-92cb-452d-bdc2-80e369f872e0
📒 Files selected for processing (6)
repository/channel.gorouter/v3/channels.gorouter/v3/router.goservice/channel/manager.goservice/channel/manager_impl.goservice/channel/mock_channel/mock_manager.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
service/channel/manager_impl.go (2)
300-309: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMissing
m.T.moveon thread renameUpdateThreadupdates the node, but never recalculatesct.paths, so a renamed thread keeps the old tree path. Callm.T.move(id, optional.Of[uuid.UUID]{}, args.Name)whenargs.Name.Validbeforem.T.updateSingle(...).🤖 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/channel/manager_impl.go` around lines 300 - 309, Update the thread rename flow around UpdateChannel and m.T.updateSingle to call m.T.move with the thread id, an empty optional parent UUID, and args.Name when args.Name.Valid is true. Perform this path recalculation before m.T.updateSingle, while preserving the existing error handling and update behavior.
266-271: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winAvoid calling
m.GetChannelwhile holdingm.T.Lock()
m.GetChannelfirst callsm.T.GetModel, which takesct.RLock(). Re-enteringm.Tunder the write lock will self-deadlock here; usem.T.isChannelPresent(ch.ParentID)likeCreateThreadChanneldoes.🤖 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/channel/manager_impl.go` around lines 266 - 271, In the thread update flow, replace the m.GetChannel call made after acquiring m.T.Lock() with the existing m.T.isChannelPresent(ch.ParentID) check, matching CreateThreadChannel’s pattern. Preserve the current error return when the parent channel is absent while avoiding re-entering m.T under its write lock.
🤖 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.
Outside diff comments:
In `@service/channel/manager_impl.go`:
- Around line 300-309: Update the thread rename flow around UpdateChannel and
m.T.updateSingle to call m.T.move with the thread id, an empty optional parent
UUID, and args.Name when args.Name.Valid is true. Perform this path
recalculation before m.T.updateSingle, while preserving the existing error
handling and update behavior.
- Around line 266-271: In the thread update flow, replace the m.GetChannel call
made after acquiring m.T.Lock() with the existing
m.T.isChannelPresent(ch.ParentID) check, matching CreateThreadChannel’s pattern.
Preserve the current error return when the parent channel is absent while
avoiding re-entering m.T under its write lock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 583acb28-dc44-4936-a8d6-be11180d75e5
📒 Files selected for processing (2)
router/v3/channels.goservice/channel/manager_impl.go
🚧 Files skipped from review as they are similar to previous changes (1)
- router/v3/channels.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
service/channel/manager_impl.go (3)
313-315: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate
ctxinto event recording.The new thread-update path calls
recordChannelEvent, which ultimately invokesRecordChannelEvent(context.Background()). This drops cancellation, deadlines, and tracing context. Passctxinto the helper and through to the repository; if recording must outlive the request, use an explicitly documented detached-context policy instead.As per path instructions, Go functions performing I/O or network requests must accept
context.Contextas their first parameter and pass the received context to subsequent calls.🤖 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/channel/manager_impl.go` around lines 313 - 315, Update the thread-update event loop and recordChannelEvent helper to accept ctx as the first parameter, then pass that context through to the repository instead of context.Background(). Preserve cancellation, deadlines, and tracing for the event-recording I/O path.Source: Path instructions
286-298: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat an unchanged name as a no-op.
isChildPresent(args.Name.V, ch.ParentID)cannot exclude the current thread, so submitting the existing name at Line 289 finds the thread itself and returnsErrChannelNameConflicts. It also records a spuriousNameChangedevent when the value is unchanged.Proposed fix
- if args.Name.Valid { + if args.Name.Valid && args.Name.V != ch.Name { if m.T.isChildPresent(args.Name.V, ch.ParentID) { return ErrChannelNameConflicts } eventRecords[model.ChannelEventNameChanged] = model.ChannelEventDetail{🤖 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/channel/manager_impl.go` around lines 286 - 298, Update the name-handling branch in the channel update method around isChildPresent to return without conflict or event recording when args.Name.V equals the current ch.Name. Only perform duplicate-name validation and add the model.ChannelEventNameChanged event when the submitted name actually differs.
260-276: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRefresh the thread after acquiring the tree lock.
The lookup at Line 261 occurs before the lock at Line 275, so concurrent updates can make
chstale. Event comparisons andbeforevalues may then be incorrect, and concurrent writes can proceed without detecting the stale state. Re-read the thread with a lock-aware lookup after acquiringm.T.Lock().🤖 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/channel/manager_impl.go` around lines 260 - 276, Update UpdateThread so it re-reads the thread through the lock-aware lookup after acquiring m.T.Lock(), handling lookup errors consistently. Use the refreshed ch for subsequent parent validation, event comparisons, and before-state values, while preserving the existing channel-type checks and lock scope.
🤖 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.
Outside diff comments:
In `@service/channel/manager_impl.go`:
- Around line 313-315: Update the thread-update event loop and
recordChannelEvent helper to accept ctx as the first parameter, then pass that
context through to the repository instead of context.Background(). Preserve
cancellation, deadlines, and tracing for the event-recording I/O path.
- Around line 286-298: Update the name-handling branch in the channel update
method around isChildPresent to return without conflict or event recording when
args.Name.V equals the current ch.Name. Only perform duplicate-name validation
and add the model.ChannelEventNameChanged event when the submitted name actually
differs.
- Around line 260-276: Update UpdateThread so it re-reads the thread through the
lock-aware lookup after acquiring m.T.Lock(), handling lookup errors
consistently. Use the refreshed ch for subsequent parent validation, event
comparisons, and before-state values, while preserving the existing channel-type
checks and lock scope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 53f1236a-f290-46d4-91ea-a1cda7ebc101
📒 Files selected for processing (2)
router/v3/router.goservice/channel/manager_impl.go
🚧 Files skipped from review as they are similar to previous changes (1)
- router/v3/router.go
4 participants
patch/threadの実装
Summary by CodeRabbit
New Features
PATCH /v3/threads/:channelID, supporting optional renaming and archived/unarchived updates in one request.Bug Fixes