feat: channel by tag omit key - #2247
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
controller/channel.go (1)
279-285: SearchChannels tag-mode: sameGetChannelsByTagusage as GetAllChannelsThis call mirrors the GetAllChannels tag-mode behavior: channels are still sorted by
idSort, andselectAll=falseavoids pullingkeyinto search results, which is desirable for safety/consistency.
🧹 Nitpick comments (1)
model/channel.go (1)
275-287: GetChannelsByTag: behavior matches intent; naming/use ofselectAllcould be clearerThe new
selectAllflag correctly controls whetherkeyis omitted while preserving the existing sort behavior. This aligns with the goal of hiding channel keys on tag-based reads.If you don’t anticipate a caller that needs the key here, you could simplify by always omitting
key(dropselectAll), or alternatively rename the flag to something likeincludeKeyto better reflect its column-level effect. This is purely a readability/naming consideration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controller/channel.go(3 hunks)model/channel.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
controller/channel.go (1)
model/channel.go (1)
GetChannelsByTag(275-287)
model/channel.go (1)
model/main.go (1)
DB(64-64)
🔇 Additional comments (3)
model/channel.go (1)
730-743: EditChannelByTag now omitskeyon reload; verifyUpdateAbilitiesdoes not depend on itSwitching
GetChannelsByTag(updatedTag, false, false)to omitkeyis consistent with avoiding unnecessary key reads. However, this assumeschannel.UpdateAbilities(nil)never relies onchannel.Keybeing populated (only on id/tag/models/group/etc.).Please double-check the implementation of
Channel.UpdateAbilitiesto confirm it doesn’t useKey; otherwise this change could introduce subtle behavior differences when recreating abilities after tag edits.controller/channel.go (2)
91-113: GetAllChannels tag-mode: now correctly omitskeyfrom tag channel listingsUsing
model.GetChannelsByTag(*tag, idSort, false)ensures tag-mode listings are consistent with the non-tag branch by not loading/exposingkeyin the returned channels. Bool ordering(tag, idSort, selectAll=false)is correct and preserves previous sort behavior.
1031-1052: GetTagModels: avoids loading keys when only models are neededHere
GetChannelsByTag(tag, false, false)is appropriate: the endpoint only derives a models string and never needskey, so omitting it reduces sensitive data exposure and unnecessary IO while keeping behavior (model selection) unchanged.
feat: channel by tag omit key
…lay-function-call-output fix: preserve replay tool output continuation
feat: channel by tag omit key
优化渠道tag返回
Summary by CodeRabbit