Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Rate limit exceeded@Southclaws has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (15)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughIntroduces a complete MyBB-to-Storyden data import system with loader modules querying MyBB MySQL tables, transform modules converting data to domain types, a writer for bulk Ent ORM creation, styled terminal logging, and a main CLI orchestrating sequential loading, transformation, and persistence phases. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI<br/>(main.go)
participant MyBB as MyBB<br/>Database
participant Loader as Loader<br/>(mybb.go)
participant Transform as Transform<br/>(roles, accounts, etc.)
participant Ent as Ent ORM<br/>(Storyden)
participant Logger as Logger
CLI->>MyBB: Open & Ping
CLI->>Loader: LoadAll(ctx, db)
loop Load phase
Loader->>MyBB: Query each table<br/>(users, forums, threads, posts, etc.)
MyBB-->>Loader: Rows
Loader->>Loader: Scan & aggregate<br/>into MyBBData
end
Loader-->>CLI: MyBBData
rect rgb(220, 240, 255)
note over CLI,Logger: Data Transformation Phase
CLI->>Transform: ImportSettings(ctx, w, data)
Transform->>Ent: CreateSettings
Ent-->>Transform: ✓
Logger-->>CLI: Log success
CLI->>Transform: ImportRoles(ctx, w, data)
Transform->>Ent: CreateRoles
Ent-->>Transform: Role IDs
Transform->>CLI: RoleIDMap
Logger-->>CLI: Log counts
CLI->>Transform: ImportAccounts(ctx, w, data)
Transform->>Transform: Hash passwords<br/>with Argon2id
Transform->>Ent: CreateAccounts<br/>CreateAuthentications<br/>CreateAccountRoles
Ent-->>Transform: Account IDs
Transform->>CLI: AccountIDMap
Logger-->>CLI: Log accounts
CLI->>Transform: ImportCategories(ctx, w, data)
Transform->>Ent: CreateCategories
Ent-->>Transform: Category IDs
Transform->>CLI: CategoryIDMap
Logger-->>CLI: Log categories
CLI->>Transform: ImportTags(ctx, w, data)
Transform->>Ent: CreateTags
Ent-->>Transform: Tag IDs
Transform->>CLI: TagIDMap
CLI->>Transform: ImportThreads(ctx, w, data)
Transform->>Transform: Convert BBCode<br/>to HTML
Transform->>Ent: CreatePosts (roots)
Ent-->>Transform: Post IDs
Transform->>CLI: PostIDMap
Logger-->>CLI: Log threads
CLI->>Transform: ImportPosts(ctx, w, data)
Transform->>Ent: CreatePosts (replies)
Ent-->>Transform: ✓
Logger-->>CLI: Log posts
CLI->>Transform: ImportInteractions(ctx, w, data)
Transform->>Ent: CreateReacts, CreateLikePosts,<br/>CreatePostReads, CreateReports
Ent-->>Transform: ✓
Logger-->>CLI: Log interactions
end
CLI->>Logger: Log completion
Logger-->>CLI: Summary message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The changes involve multiple interdependent modules with varying logic density. The transform cohort contains intricate business logic (password hashing, permission mapping, BBCode conversion, nested data structure handling), while the loader and writer are more mechanical but comprehensive. The main.go file orchestrates a multi-phase control flow requiring understanding of the full import pipeline. New domain knowledge is required (MyBB schema, role/permission mapping, account creation flows), and error propagation is consistent but widespread. Configuration and documentation aspects are minimal, but heterogeneous changes across logger, loader, transform, and writer require context-switching and reasoning per cohort. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
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 |
Southclaws
force-pushed
the
import-mybb
branch
2 times, most recently
from
0aff8d8 to
08309c9
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (10)
cmd/import-mybb/transform/bbcode.go (1)
131-135: Consider logging sanitization failures.When
datagraph.NewRichTextfails, the function silently returns an emptyContent{}, which could result in data loss for posts without any indication of what went wrong during the import.🔎 Proposed improvement
// Use NewRichText to sanitize and generate short summary content, err := datagraph.NewRichText(html) if err != nil { - // If sanitization fails, return empty content - // This shouldn't normally happen but handle it gracefully + // Log the sanitization failure so it can be investigated + log.Printf("Warning: BBCode sanitization failed for input (first 100 chars): %s... Error: %v", + truncateForLog(input, 100), err) return datagraph.Content{} }You'll need to add a simple helper or use an existing truncation utility for the log message.
cmd/import-mybb/transform/interactions.go (2)
196-199: Consider logging skipped report types for visibility.Non-"profile" reports are silently skipped. Adding a log or counter for skipped reports would help operators understand what data wasn't imported.
238-241: Stub acknowledged - asset import placeholder.The TODO is clear. Consider adding a more detailed comment about what filesystem access is needed, or linking to a tracking issue.
Would you like me to help create a GitHub issue to track the asset import implementation?
cmd/import-mybb/main.go (1)
64-67: Destructive schema migration with drop options.
WithDropColumn(true)andWithDropIndex(true)can cause data loss. While this may be intentional for a clean import, consider adding a confirmation prompt or requiring an explicit flag like--forcefor production safety.cmd/import-mybb/transform/roles.go (1)
56-69: Role name matching may be too strict.The switch matches exact lowercase titles like "guests", "members", "admin". MyBB installations often use variations like "Administrators", "Registered", "Super Moderators", etc. Consider adding more aliases or using contains-based matching.
🔎 Proposed enhancement
func mapDefaultRoleID(title string) (xid.ID, bool) { normalised := strings.ToLower(strings.TrimSpace(title)) switch normalised { - case "guests": + case "guests", "guest", "unregistered": return xid.ID(role.DefaultRoleGuestID), true - case "members": + case "members", "member", "registered", "users": return xid.ID(role.DefaultRoleMemberID), true - case "admin": + case "admin", "admins", "administrators", "administrator": return xid.ID(role.DefaultRoleAdminID), true default: return xid.ID{}, false } }cmd/import-mybb/transform/accounts.go (2)
87-87: Remove commented-out code.This appears to be debug remnant. Either re-enable the logging or remove the line entirely.
253-278: Consider pre-building a group permissions map for better performance.
checkUserIsAdminiterates through all user groups for each user. For large imports, pre-building amap[int]boolof admin-capable groups would reduce complexity from O(users × groups) to O(users + groups).🔎 Optional optimization
// Pre-build at start of ImportAccounts: adminGroups := make(map[int]bool) for _, group := range data.UserGroups { if group.CanCP == 1 { adminGroups[group.GID] = true } } // Then in the loop: isAdmin := adminGroups[user.UserGroup] if !isAdmin && user.AdditionalGroups != "" { // check additional groups against adminGroups map }cmd/import-mybb/transform/posts.go (1)
166-168: Redundant nil check after successful map lookup.At line 140-144, you already verified
rootPostIDexists in the map and continue only on success. Thexid.NilID()check here is therefore unnecessary since the map lookup guarantees a valid ID was found.🔎 Proposed simplification
- if rootPostID != xid.NilID() { - builder.SetRootPostID(rootPostID) - } + builder.SetRootPostID(rootPostID)cmd/import-mybb/writer/storyden.go (2)
24-34:FirstPostPIDsmap not initialized in constructor.The
FirstPostPIDsfield is not initialized inNew(), which could cause a nil map panic if code attempts to read from it beforeImportThreadssets it. While currently the flow appears safe, defensive initialization would prevent potential runtime panics if the import order changes.🔎 Proposed fix
func New(client *ent.Client, batchSize int) *Writer { return &Writer{ client: client, batchSize: batchSize, RoleIDMap: make(map[int]xid.ID), AccountIDMap: make(map[int]xid.ID), CategoryIDMap: make(map[int]xid.ID), PostIDMap: make(map[int]xid.ID), TagIDMap: make(map[int]xid.ID), + FirstPostPIDs: make(map[int]bool), } }
256-265: Inconsistent batching strategy for bulk creates.
CreateReacts,CreateLikePosts,CreatePostReads,CreateReports, andCreateAssetsdon't use batching, while methods likeCreatePostsandCreateAccountsdo. For large imports, this could cause memory or transaction size issues.Consider applying the same batching pattern to these methods if the imported datasets could be large, or document that these are expected to remain small.
Also applies to: 267-276, 278-287, 289-298, 300-309
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (15)
cmd/import-mybb/loader/mybb.gocmd/import-mybb/loader/types.gocmd/import-mybb/logger/logger.gocmd/import-mybb/logger/styles.gocmd/import-mybb/main.gocmd/import-mybb/transform/accounts.gocmd/import-mybb/transform/bbcode.gocmd/import-mybb/transform/categories.gocmd/import-mybb/transform/interactions.gocmd/import-mybb/transform/posts.gocmd/import-mybb/transform/roles.gocmd/import-mybb/transform/settings.gocmd/import-mybb/transform/tags.gocmd/import-mybb/writer/storyden.gointernal/infrastructure/db/db.go
🧰 Additional context used
📓 Path-based instructions (1)
internal/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Go internal/ must NEVER import anything from ./app/
Files:
internal/infrastructure/db/db.go
🧬 Code graph analysis (9)
cmd/import-mybb/transform/tags.go (4)
cmd/import-mybb/writer/storyden.go (2)
Writer(12-22)New(24-34)cmd/import-mybb/loader/types.go (1)
MyBBData(5-22)internal/ent/tag_create.go (1)
TagCreate(23-28)cmd/import-mybb/logger/logger.go (1)
Tag(93-100)
cmd/import-mybb/transform/bbcode.go (1)
app/resources/datagraph/content.go (1)
NewRichText(140-142)
cmd/import-mybb/transform/interactions.go (7)
cmd/import-mybb/writer/storyden.go (1)
Writer(12-22)cmd/import-mybb/loader/types.go (1)
MyBBData(5-22)internal/ent/react_create.go (1)
ReactCreate(22-27)internal/ent/likepost_create.go (1)
LikePostCreate(22-27)internal/ent/postread_create.go (1)
PostReadCreate(22-27)internal/ent/report/where.go (1)
Reason(95-97)app/resources/datagraph/datagraph_enum_gen.go (1)
KindProfile(20-20)
cmd/import-mybb/logger/logger.go (1)
cmd/import-mybb/logger/styles.go (12)
HeaderStyle(18-24)SuccessStyle(27-31)ErrorStyle(34-38)AccountLabel(41-45)FieldKey(72-74)FieldValue(76-78)Arrow(81-83)CategoryLabel(47-51)Dim(86-87)PostLabel(53-57)RoleLabel(59-63)TagLabel(65-69)
cmd/import-mybb/loader/mybb.go (1)
cmd/import-mybb/loader/types.go (16)
MyBBData(5-22)MyBBUserField(44-47)MyBBUser(24-42)MyBBUserGroup(49-68)MyBBForum(70-79)MyBBThread(81-96)MyBBPost(98-109)MyBBThreadPrefix(111-114)MyBBReputation(116-123)MyBBThreadRating(125-130)MyBBThreadRead(132-136)MyBBReportedContent(138-145)MyBBBanned(147-152)MyBBAttachment(154-160)MyBBProfileField(162-167)MyBBUserTitle(169-173)
cmd/import-mybb/transform/categories.go (4)
cmd/import-mybb/writer/storyden.go (2)
Writer(12-22)New(24-34)cmd/import-mybb/loader/types.go (1)
MyBBData(5-22)cmd/import-mybb/logger/logger.go (2)
Skip(108-111)Category(43-52)app/resources/mark/slugify.go (1)
Slugify(17-37)
cmd/import-mybb/transform/settings.go (3)
cmd/import-mybb/writer/storyden.go (2)
Writer(12-22)New(24-34)cmd/import-mybb/loader/types.go (1)
MyBBData(5-22)app/resources/account/authentication/authentication_enum_gen.go (1)
ModeEmail(16-16)
cmd/import-mybb/transform/accounts.go (5)
cmd/import-mybb/writer/storyden.go (2)
Writer(12-22)New(24-34)cmd/import-mybb/loader/types.go (2)
MyBBBanned(147-152)MyBBUser(24-42)app/resources/mark/slugify.go (1)
Slugify(17-37)cmd/import-mybb/logger/logger.go (2)
Info(103-105)Account(25-40)app/resources/account/role/default.go (2)
DefaultRoleGuestID(13-13)DefaultRoleMemberID(14-14)
cmd/import-mybb/writer/storyden.go (11)
cmd/import-mybb/logger/logger.go (5)
Success(15-17)Account(25-40)Role(77-90)Category(43-52)Tag(93-100)internal/ent/account_create.go (1)
AccountCreate(41-46)internal/ent/authentication_create.go (1)
AuthenticationCreate(21-26)internal/ent/accountroles_create.go (1)
AccountRolesCreate(22-27)internal/ent/email_create.go (1)
EmailCreate(21-26)internal/ent/tag_create.go (1)
TagCreate(23-28)internal/ent/post_create.go (1)
PostCreate(31-36)internal/ent/react_create.go (1)
ReactCreate(22-27)internal/ent/likepost_create.go (1)
LikePostCreate(22-27)internal/ent/postread_create.go (1)
PostReadCreate(22-27)internal/ent/asset_create.go (1)
AssetCreate(25-30)
🔇 Additional comments (18)
internal/infrastructure/db/db.go (1)
40-40: LGTM! Clean export of database connection functions.The changes appropriately export
NewSQLandConnectfunctions to make them available for the import CLI tool. The logic remains unchanged, and the code complies with the coding guideline thatinternal/**/*.gomust not import from./app/.Also applies to: 47-64, 73-73, 151-178
cmd/import-mybb/loader/types.go (1)
1-180: LGTM! Well-structured data types for MyBB import.The type definitions are comprehensive and appropriate for the MyBB data model. The
unixToTimehelper correctly handles zero timestamps by returning the zero time value.cmd/import-mybb/logger/styles.go (1)
1-88: LGTM! Consistent and well-organized styling definitions.The color palette and style definitions are well-structured. The consistent
Width(12)and right alignment across all resource labels ensures clean terminal output formatting.cmd/import-mybb/logger/logger.go (1)
1-119: LGTM! Clean and well-structured logging helpers.The logging functions are well-organized and make good use of the styling definitions from
styles.go. Thetruncatehelper correctly handles the edge case where the string length equalsmaxLenby returning it unchanged.cmd/import-mybb/transform/tags.go (1)
1-47: LGTM - Clean implementation of tag import.The function correctly handles empty inputs, skips empty prefixes, maintains ID mappings for cross-referencing, and uses proper error wrapping. The pre-allocated slice capacity is a good optimization.
cmd/import-mybb/transform/settings.go (1)
25-29: Consider whether DefaultSettings requires a deep copy.If
settings.DefaultSettingscontains pointer or reference types, the direct assignment creates a shallow copy. Modifications here could affect the default. Verify thatopt.New()creates independent values.cmd/import-mybb/transform/roles.go (1)
115-118: Collection permissions granted unconditionally to all roles.Lines 116-117 add collection permissions outside any conditional, meaning even guest/restricted roles get collection access. Verify this is the intended behavior.
cmd/import-mybb/transform/accounts.go (2)
130-139: Handle missing DisplayGroup gracefully.If
user.DisplayGroupis 0 or not present inRoleIDMap,displayGroupIDwill be the zero-valuexid.ID. The comparison on line 136 could then incorrectly match a role with a zero ID, though this is unlikely given xid generation.🔎 Safer approach
- displayGroupID := w.RoleIDMap[user.DisplayGroup] - badgeAssigned := false + displayGroupID, hasDisplayGroup := w.RoleIDMap[user.DisplayGroup] + badgeAssigned := !hasDisplayGroup for _, roleID := range roleIDs { badge := false - if !badgeAssigned && roleID == displayGroupID { + if !badgeAssigned && hasDisplayGroup && roleID == displayGroupID { badge = true
179-203: Bio content may contain raw BBCode.
user.UserTitleanduser.Signaturelikely contain BBCode from MyBB. Consider whether these should be converted to plain text or HTML using the BBCode converter from this PR.cmd/import-mybb/transform/interactions.go (1)
104-107: No issues found. ThePostIDMapis correctly designed to store both thread IDs (mapping to root posts) and post IDs as keys, so the lookup at line 104 usingrating.TIDis intentional and functional.cmd/import-mybb/transform/posts.go (3)
18-116: ImportThreads implementation looks solid.The function properly builds a post map for O(1) lookups, tracks first-post PIDs for downstream use, handles visibility mapping, and associates threads with categories and tags. The use of
xid.New()for generating IDs and storing them in maps for cross-referencing is a clean approach.
118-185: ImportPosts handles reply creation correctly.Good separation of concerns by skipping first-posts (already imported as root posts) and proper error handling with descriptive skip messages. The logging of skipped first posts count is helpful for debugging.
187-200: Visibility mapping is clear and well-documented.The switch covers all MyBB visibility states with sensible mappings. The inline comment for
-1(unapproved/awaiting moderation) and-2(soft deleted) aids maintainability.cmd/import-mybb/loader/mybb.go (2)
9-98: LoadAll orchestration is clean and well-structured.The sequential loading with proper error wrapping provides good context on failure. Each loader is independent and the aggregation into
MyBBDatais straightforward.
100-122: Loader functions follow a consistent, idiomatic pattern.Good use of
defer rows.Close()for resource cleanup, proper error propagation viarows.Err(), and clear column-to-struct field mappings.Also applies to: 142-168, 170-189, 191-212, 214-234, 236-255, 257-276, 278-297, 299-318, 320-339, 363-382, 384-403, 405-424, 426-446
cmd/import-mybb/writer/storyden.go (3)
12-22: Well-designed Writer struct for maintaining import state.The ID maps provide a clean mechanism for cross-referencing MyBB IDs to Storyden XIDs across different entity types. This design enables proper relationship mapping during the import process.
128-137: Bulk create methods are well-implemented with proper batching where applied.The batching logic correctly handles edge cases (last batch smaller than batch size) and accumulates results properly. Success logging provides good visibility into import progress.
Also applies to: 139-156, 158-175, 177-194, 196-213, 215-224, 226-235, 237-254
44-126: No issues found with deletion order.The deletion order is safe because the schema uses
OnDelete(Cascade)constraints appropriately:
- Deleting
Accountcascades to deleteAccountRolesandAuthentication(lines 86-89, 91-92 in account.go)- Deleting
Postfirst is safe since the foreign key toCategoryis optional (category_id can be null in post.go line 42)- No foreign key constraint violations will occur with the current order
| func loadUserFields(ctx context.Context, db *sql.DB) (map[int]MyBBUserField, error) { | ||
| rows, err := db.QueryContext(ctx, `SELECT ufid FROM mybb_userfields`) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer rows.Close() | ||
|
|
||
| fields := make(map[int]MyBBUserField) | ||
| for rows.Next() { | ||
| var ufid int | ||
| if err := rows.Scan(&ufid); err != nil { | ||
| return nil, err | ||
| } | ||
| fields[ufid] = MyBBUserField{UFID: ufid, Fields: make(map[string]string)} | ||
| } | ||
| return fields, rows.Err() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the MyBBUserField struct definition
rg -n 'type MyBBUserField struct' -A 5 --type=goRepository: Southclaws/storyden
Length of output: 369
🏁 Script executed:
# Check how UserFields is used in the codebase
rg -n 'UserFields|\.Fields' --type=go -B 2 -A 2Repository: Southclaws/storyden
Length of output: 50376
🏁 Script executed:
# Look at the LoadAll function to see how loadUserFields result is used
sed -n '9,98p' cmd/import-mybb/loader/mybb.go | cat -nRepository: Southclaws/storyden
Length of output: 2841
🏁 Script executed:
# Check if there's any transform/processing of the UserFields data
rg -n 'loadUserFields\|mybbfields\|Fields' cmd/import-mybb/ --type=go -B 2 -A 2Repository: Southclaws/storyden
Length of output: 45
🏁 Script executed:
# Find files that import or reference the loader package
fd -e go . cmd/import-mybb/ | head -20Repository: Southclaws/storyden
Length of output: 548
🏁 Script executed:
# Look for transform or processing of loaded MyBBData
rg -l 'MyBBData|loadUserFields|UserFields' cmd/import-mybb/ --type=goRepository: Southclaws/storyden
Length of output: 387
🏁 Script executed:
# Check the main import logic file
cat cmd/import-mybb/main.goRepository: Southclaws/storyden
Length of output: 4745
🏁 Script executed:
# Check if UserFields is used anywhere in transform files
rg 'UserFields' cmd/import-mybb/transform/ --type=go -B 2 -A 2Repository: Southclaws/storyden
Length of output: 407
🏁 Script executed:
# Check the ProfileFields loading since that might be related
rg -n 'loadProfileFields' cmd/import-mybb/loader/mybb.go -A 20Repository: Southclaws/storyden
Length of output: 1224
🏁 Script executed:
# Check what the actual user fields table structure looks like
rg -n 'mybb_userfields' cmd/import-mybb/ --type=go -B 5 -A 5Repository: Southclaws/storyden
Length of output: 695
🏁 Script executed:
# Check the exact usage in accounts.go to understand the context better
rg -n 'UserFields' cmd/import-mybb/transform/accounts.go -B 5 -A 10Repository: Southclaws/storyden
Length of output: 465
🏁 Script executed:
# Check MyBBUserField struct and related structures more thoroughly
sed -n '40,60p' cmd/import-mybb/loader/types.go | cat -nRepository: Southclaws/storyden
Length of output: 629
loadUserFields loads only the field ID metadata but never populates the actual field values.
The function creates an empty MyBBUserField.Fields map for each user field ID but never queries the actual field name/value data. Code in cmd/import-mybb/transform/accounts.go (lines 190–196) attempts to use this data to populate user profile descriptions, but the condition len(userFields.Fields) > 0 will always be false since the map is never populated.
The current query SELECT ufid FROM mybb_userfields only loads field metadata. To complete the implementation, the function should also query the actual user field values (typically stored in user data rows as dynamic columns like fid1, fid2, etc., or in a related userfield values table).
🤖 Prompt for AI Agents
In cmd/import-mybb/loader/mybb.go around lines 124 to 140, loadUserFields
currently only fetches ufid and initializes an empty Fields map, so downstream
code never sees any field name/value pairs; update this function to populate
MyBBUserField.Fields by querying the actual field definitions and values (e.g.,
join mybb_userfields with the table that contains field names/values or run a
second query to select field name and associated value columns), iterate the
result set and assign each field name -> value into the Fields map for the
corresponding ufid, and ensure rows and errors are handled and returned as
before.
| func loadBanned(ctx context.Context, db *sql.DB) ([]MyBBBanned, error) { | ||
| rows, err := db.QueryContext(ctx, ` | ||
| SELECT uid, gid, dateline, reason, lifted | ||
| FROM mybb_banned | ||
| `) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer rows.Close() | ||
|
|
||
| var banned []MyBBBanned | ||
| for rows.Next() { | ||
| var b MyBBBanned | ||
| var lifted int | ||
| if err := rows.Scan(&b.UID, &b.GID, &b.DateBan, &b.Reason, &lifted); err != nil { | ||
| return nil, err | ||
| } | ||
| banned = append(banned, b) | ||
| } | ||
| return banned, rows.Err() | ||
| } |
There was a problem hiding this comment.
Unused variable lifted in loadBanned.
The lifted variable is scanned from the query result but never used. Either remove it from the query and scan, or add a Lifted field to MyBBBanned if the data is needed.
🔎 Option 1: Remove from query if not needed
func loadBanned(ctx context.Context, db *sql.DB) ([]MyBBBanned, error) {
rows, err := db.QueryContext(ctx, `
- SELECT uid, gid, dateline, reason, lifted
+ SELECT uid, gid, dateline, reason
FROM mybb_banned
`)
if err != nil {
return nil, err
}
defer rows.Close()
var banned []MyBBBanned
for rows.Next() {
var b MyBBBanned
- var lifted int
- if err := rows.Scan(&b.UID, &b.GID, &b.DateBan, &b.Reason, &lifted); err != nil {
+ if err := rows.Scan(&b.UID, &b.GID, &b.DateBan, &b.Reason); err != nil {
return nil, err
}
banned = append(banned, b)
}
return banned, rows.Err()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func loadBanned(ctx context.Context, db *sql.DB) ([]MyBBBanned, error) { | |
| rows, err := db.QueryContext(ctx, ` | |
| SELECT uid, gid, dateline, reason, lifted | |
| FROM mybb_banned | |
| `) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer rows.Close() | |
| var banned []MyBBBanned | |
| for rows.Next() { | |
| var b MyBBBanned | |
| var lifted int | |
| if err := rows.Scan(&b.UID, &b.GID, &b.DateBan, &b.Reason, &lifted); err != nil { | |
| return nil, err | |
| } | |
| banned = append(banned, b) | |
| } | |
| return banned, rows.Err() | |
| } | |
| func loadBanned(ctx context.Context, db *sql.DB) ([]MyBBBanned, error) { | |
| rows, err := db.QueryContext(ctx, ` | |
| SELECT uid, gid, dateline, reason | |
| FROM mybb_banned | |
| `) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer rows.Close() | |
| var banned []MyBBBanned | |
| for rows.Next() { | |
| var b MyBBBanned | |
| if err := rows.Scan(&b.UID, &b.GID, &b.DateBan, &b.Reason); err != nil { | |
| return nil, err | |
| } | |
| banned = append(banned, b) | |
| } | |
| return banned, rows.Err() | |
| } |
🤖 Prompt for AI Agents
In cmd/import-mybb/loader/mybb.go around lines 341 to 361 the variable `lifted`
is scanned from the SQL result but never used; either remove the `lifted` column
from the SELECT and from the rows.Scan call if the lifted timestamp/state is not
needed, or add a `Lifted` field to the MyBBBanned struct (with appropriate type,
e.g., int or time.Time) and assign the scanned value to that field (update
struct definition and any consumers) so the scanned value is used.
| // [video=youtube] - YouTube embeds converted to links (iframes are sanitized out) | ||
| bbcodeCompiler.SetTag("video", func(node *bbcode.BBCodeNode) (*bbcode.HTMLTag, bool) { | ||
| out := bbcode.NewHTMLTag("") | ||
| out.Name = "a" | ||
|
|
||
| videoURL := bbcode.CompileText(node) | ||
| videoType := node.GetOpeningTag().Value | ||
|
|
||
| if videoType == "youtube" { | ||
| // Extract video ID from various YouTube URL formats | ||
| // Could be: https://www.youtube.com/watch?v=VIDEO_ID or just VIDEO_ID | ||
| videoID := videoURL | ||
| if len(videoURL) > 20 { | ||
| // It's a full URL, try to extract the ID | ||
| if idx := len(videoURL) - 11; idx >= 0 && len(videoURL) >= 11 { | ||
| // YouTube video IDs are typically 11 characters | ||
| videoID = videoURL[len(videoURL)-11:] | ||
| } | ||
| } | ||
| out.Attrs["href"] = "https://www.youtube.com/watch?v=" + videoID | ||
| out.Value = "YouTube Video: " + videoID | ||
| } else { | ||
| // Fallback for unknown video types | ||
| out.Attrs["href"] = videoURL | ||
| out.Value = "Video: " + videoURL | ||
| } | ||
|
|
||
| return out, false | ||
| }) |
There was a problem hiding this comment.
Fix the YouTube video ID extraction logic.
The current video ID extraction logic is overly simplistic and will fail for most YouTube URL formats:
- Standard URLs like
https://www.youtube.com/watch?v=VIDEO_IDwill extract the last 11 characters, which would be=VIDEO_ID(including the=from the query parameter) - Short URLs like
https://youtu.be/VIDEO_IDwon't work correctly - URLs with additional query parameters (e.g.,
&t=30s) will extract parts of those parameters instead of the video ID
🔎 Proposed fix using proper URL parsing
// [video=youtube] - YouTube embeds converted to links (iframes are sanitized out)
bbcodeCompiler.SetTag("video", func(node *bbcode.BBCodeNode) (*bbcode.HTMLTag, bool) {
out := bbcode.NewHTMLTag("")
out.Name = "a"
videoURL := bbcode.CompileText(node)
videoType := node.GetOpeningTag().Value
if videoType == "youtube" {
- // Extract video ID from various YouTube URL formats
- // Could be: https://www.youtube.com/watch?v=VIDEO_ID or just VIDEO_ID
- videoID := videoURL
- if len(videoURL) > 20 {
- // It's a full URL, try to extract the ID
- if idx := len(videoURL) - 11; idx >= 0 && len(videoURL) >= 11 {
- // YouTube video IDs are typically 11 characters
- videoID = videoURL[len(videoURL)-11:]
- }
- }
+ // Extract video ID from various YouTube URL formats
+ videoID := extractYouTubeID(videoURL)
out.Attrs["href"] = "https://www.youtube.com/watch?v=" + videoID
out.Value = "YouTube Video: " + videoID
} else {
// Fallback for unknown video types
out.Attrs["href"] = videoURL
out.Value = "Video: " + videoURL
}
return out, false
})
+
+func extractYouTubeID(videoURL string) string {
+ // If it's already just an ID (11 chars, no special chars), return as-is
+ if len(videoURL) == 11 && !strings.ContainsAny(videoURL, "/:?&=") {
+ return videoURL
+ }
+
+ // Try to parse as URL
+ if strings.Contains(videoURL, "youtube.com") || strings.Contains(videoURL, "youtu.be") {
+ // Handle youtu.be short URLs
+ if strings.Contains(videoURL, "youtu.be/") {
+ parts := strings.Split(videoURL, "youtu.be/")
+ if len(parts) > 1 {
+ id := strings.Split(parts[1], "?")[0]
+ if len(id) == 11 {
+ return id
+ }
+ }
+ }
+ // Handle youtube.com URLs with v= parameter
+ if strings.Contains(videoURL, "v=") {
+ parts := strings.Split(videoURL, "v=")
+ if len(parts) > 1 {
+ id := strings.Split(parts[1], "&")[0]
+ if len(id) == 11 {
+ return id
+ }
+ }
+ }
+ }
+
+ // Fallback: return as-is if we can't parse it
+ return videoURL
+}Note: You'll need to add "strings" to the imports.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/import-mybb/transform/bbcode.go around lines 52 to 80, the YouTube ID
extraction is brittle (slicing last 11 chars) and fails for common URL forms;
replace it with proper URL parsing: use net/url to Parse the videoURL, handle
hostnames like "youtu.be" (video ID is the first path segment), "youtube.com" or
"www.youtube.com" with path "/watch" (video ID from query param "v") and
"/embed/" (ID from the path segment), strip any query parameters or fragments,
and if parsing fails treat the original string as a potential raw ID and
validate/trim to 11 chars; update the href to
"https://www.youtube.com/watch?v="+id and add "strings" (and "net/url") to
imports as needed.
| for _, forum := range data.Forums { | ||
| if forum.Active == 0 { | ||
| logger.Skip("category", fmt.Sprintf("inactive forum: %s", forum.Name)) | ||
| continue | ||
| } | ||
|
|
||
| id := xid.New() | ||
| w.CategoryIDMap[forum.FID] = id | ||
|
|
||
| parentID := parseParentFromList(forum.ParentList, w) | ||
| slug := mark.Slugify(forum.Name) | ||
|
|
||
| builder := w.Client().Category.Create(). | ||
| SetID(id). | ||
| SetName(forum.Name). | ||
| SetSlug(slug). | ||
| SetSort(forum.DispOrder) | ||
|
|
||
| if forum.Description != "" { | ||
| builder.SetDescription(forum.Description) | ||
| } | ||
|
|
||
| if !parentID.IsNil() { | ||
| builder.SetParentCategoryID(parentID) | ||
| } | ||
|
|
||
| builders = append(builders, builder) | ||
|
|
||
| logger.Category(forum.FID, forum.Name, slug) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for ORDER BY clauses related to forums in loader
rg -n "ORDER BY|order by" cmd/import-mybb/loader/ -i -A2 -B2
# Look for forum query/fetch logic
rg -n "Forums\s*=|Forums\s*\[|SELECT.*forum" cmd/import-mybb/loader/ -i -A5 -B2
# Check the loader package structure
ls -la cmd/import-mybb/loader/Repository: Southclaws/storyden
Length of output: 1695
🏁 Script executed:
# Get the full loadForums function to see the SQL query
sed -n '170,190p' cmd/import-mybb/loader/mybb.goRepository: Southclaws/storyden
Length of output: 599
🏁 Script executed:
# Check the parseParentFromList function to understand how parent lookup fails
sed -n '65,84p' cmd/import-mybb/transform/categories.goRepository: Southclaws/storyden
Length of output: 480
🏁 Script executed:
# Check if there's any sorting or reordering of forums after loading
rg -n "sort|Sort" cmd/import-mybb/transform/categories.go -i
# Check if there's any post-processing that fixes parent relationships
rg -n "CategoryIDMap\|SetParentCategoryID" cmd/import-mybb/transform/ -A3 -B3
# Look at the full ImportCategories function to understand the complete flow
sed -n '1,63p' cmd/import-mybb/transform/categories.goRepository: Southclaws/storyden
Length of output: 1552
🏁 Script executed:
# Check the MyBBForum type definition
sed -n '1,50p' cmd/import-mybb/loader/types.go
# Look for any other forum processing or sorting
rg -n "data.Forums\|Forums\[" cmd/import-mybb/ -A5 -B2Repository: Southclaws/storyden
Length of output: 1211
🏁 Script executed:
# Get the full MyBBForum type definition
rg -n "type MyBBForum" cmd/import-mybb/loader/types.go -A20
# Check if PID field indicates direct parent, and look at actual forum structure
cat -n cmd/import-mybb/loader/types.go | grep -A30 "type MyBBForum"Repository: Southclaws/storyden
Length of output: 1209
🏁 Script executed:
# Check if there's any post-processing or sorting of forums after loading
rg -n "Load\|Import" cmd/import-mybb/ | grep -i "func\|main"
# Look at the main import flow to see if forums are sorted anywhere
rg -n "ImportCategories\|loadForums\|data.Forums" cmd/import-mybb/ -B2 -A2
# Check the main entry point
fd -n "main.go" cmd/import-mybb/Repository: Southclaws/storyden
Length of output: 293
🏁 Script executed:
# Find main entry point
find cmd/import-mybb -name "main.go"
# Look for the main function and import flow
rg -n "func main" cmd/import-mybb/ -A30
# Check how LoadData is called and if forums are processed in order
rg -n "LoadData\|ImportCategories" cmd/import-mybb/ -B3 -A3Repository: Southclaws/storyden
Length of output: 2104
🏁 Script executed:
# Get the rest of the run function to see how data flows
sed -n '40,120p' cmd/import-mybb/main.goRepository: Southclaws/storyden
Length of output: 2802
Fix forum ordering to ensure parent categories are processed before children.
The loadForums function loads forums from the database with no ORDER BY clause, so they arrive in arbitrary order. When ImportCategories iterates through forums sequentially, it populates w.CategoryIDMap as it goes. If a child forum appears before its parent, the parseParentFromList lookup will fail to find the parent ID (which hasn't been added to the map yet) and silently return NilID. This breaks the category hierarchy without any error.
Either sort forums by hierarchy depth before import, perform multiple passes until all parents resolve, or add an ORDER BY clause to the query to guarantee parent ordering.
🤖 Prompt for AI Agents
In cmd/import-mybb/transform/categories.go around lines 25-54, the loop
processes forums in arbitrary order so children can be handled before their
parents, causing parseParentFromList to return NilID; fix by ensuring parents
are created before children — e.g., before the loop sort data.Forums by
hierarchy depth (compute depth from forum.ParentList by counting separators or
parent IDs, ascending) or modify the DB query in loadForums to ORDER BY a
parent-first criterion; alternatively implement multi-pass resolution that
defers builders whose parent is missing and retries until all parents resolve.
Ensure the chosen approach guarantees parent IDs are present in w.CategoryIDMap
before parsing child ParentList.
| parentID := parseParentFromList(forum.ParentList, w) | ||
| slug := mark.Slugify(forum.Name) | ||
|
|
||
| builder := w.Client().Category.Create(). | ||
| SetID(id). | ||
| SetName(forum.Name). | ||
| SetSlug(slug). | ||
| SetSort(forum.DispOrder) |
There was a problem hiding this comment.
Handle empty slugs from category names.
Unlike the account import (which generates a fallback handle), category import doesn't handle the case where mark.Slugify(forum.Name) returns an empty string. This could occur with forum names containing only special characters or emojis.
🔎 Proposed fix
slug := mark.Slugify(forum.Name)
+ if slug == "" {
+ slug = fmt.Sprintf("category-%d", forum.FID)
+ logger.Info(fmt.Sprintf("Generated fallback slug for forum '%s' (fid:%d) → %s", forum.Name, forum.FID, slug))
+ }
builder := w.Client().Category.Create().📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| parentID := parseParentFromList(forum.ParentList, w) | |
| slug := mark.Slugify(forum.Name) | |
| builder := w.Client().Category.Create(). | |
| SetID(id). | |
| SetName(forum.Name). | |
| SetSlug(slug). | |
| SetSort(forum.DispOrder) | |
| parentID := parseParentFromList(forum.ParentList, w) | |
| slug := mark.Slugify(forum.Name) | |
| if slug == "" { | |
| slug = fmt.Sprintf("category-%d", forum.FID) | |
| logger.Info(fmt.Sprintf("Generated fallback slug for forum '%s' (fid:%d) → %s", forum.Name, forum.FID, slug)) | |
| } | |
| builder := w.Client().Category.Create(). | |
| SetID(id). | |
| SetName(forum.Name). | |
| SetSlug(slug). | |
| SetSort(forum.DispOrder) |
🤖 Prompt for AI Agents
In cmd/import-mybb/transform/categories.go around lines 34 to 41,
mark.Slugify(forum.Name) can return an empty string for names made of only
punctuation/emoji; detect when slug == "" and replace it with a deterministic
fallback (for example "category-<id>" or a sanitized version of the parent/name
if available) before calling SetSlug; ensure the fallback is lowercased and
URL-safe and use the category id to guarantee uniqueness.
| postID, ok := w.PostIDMap[read.TID] | ||
| if !ok { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Same TID vs PID issue for thread reads.
Similar to importLikes, this uses read.TID (thread ID) to look up in PostIDMap. This will likely fail silently for the same reason.
🤖 Prompt for AI Agents
In cmd/import-mybb/transform/interactions.go around lines 152 to 155, the code
looks up PostIDMap using read.TID (thread ID) which is incorrect for post-level
reads; change the lookup to use the post identifier (read.PID) or, if the intent
is to map threads, use the appropriate ThreadIDMap. Update the conditional to
retrieve postID using read.PID and keep the same continue-on-miss behavior so
reads without a mapped post are still skipped.
| createdAt := time.Unix(firstPost.DateLine, 0) | ||
| lastReplyAt := time.Unix(thread.LastPost, 0) | ||
| if lastReplyAt.IsZero() { | ||
| lastReplyAt = createdAt | ||
| } |
There was a problem hiding this comment.
Incorrect zero-time check for lastReplyAt.
time.Unix(0, 0) returns 1970-01-01 00:00:00 UTC, not the zero value of time.Time. The IsZero() method checks for time.Time{} (year 0001), so this condition will never be true when thread.LastPost == 0.
🔎 Proposed fix
createdAt := time.Unix(firstPost.DateLine, 0)
- lastReplyAt := time.Unix(thread.LastPost, 0)
- if lastReplyAt.IsZero() {
+ lastReplyAt := createdAt
+ if thread.LastPost > 0 {
+ lastReplyAt = time.Unix(thread.LastPost, 0)
+ } else if thread.LastPost < firstPost.DateLine {
lastReplyAt = createdAt
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| createdAt := time.Unix(firstPost.DateLine, 0) | |
| lastReplyAt := time.Unix(thread.LastPost, 0) | |
| if lastReplyAt.IsZero() { | |
| lastReplyAt = createdAt | |
| } | |
| createdAt := time.Unix(firstPost.DateLine, 0) | |
| lastReplyAt := createdAt | |
| if thread.LastPost > 0 { | |
| lastReplyAt = time.Unix(thread.LastPost, 0) | |
| } else if thread.LastPost < firstPost.DateLine { | |
| lastReplyAt = createdAt | |
| } |
🤖 Prompt for AI Agents
In cmd/import-mybb/transform/posts.go around lines 62 to 66, the code checks
lastReplyAt.IsZero() but time.Unix(0,0) is 1970-01-01 and IsZero() will never be
true; change the check to inspect the original unix value (e.g., if
thread.LastPost == 0) or compare lastReplyAt.Unix() == 0, and if true set
lastReplyAt = createdAt so threads with LastPost==0 fall back to the created
time.
| _, err = w.Client().Setting.Get(ctx, StorydenPrimarySettingsKey) | ||
| if err == nil { | ||
| // Update existing settings | ||
| _, err = w.Client().Setting.UpdateOneID(StorydenPrimarySettingsKey). | ||
| SetValue(string(settingsJSON)). | ||
| SetUpdatedAt(time.Now()). | ||
| Save(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("update settings: %w", err) | ||
| } | ||
| log.Printf("Updated Storyden settings with MyBB forum name: %s", data.Settings["bbname"]) | ||
| } else { |
There was a problem hiding this comment.
Distinguish "not found" from other errors when checking for existing settings.
The current logic treats any error from Get as "not found" and falls through to Create. This could mask real errors (network issues, permission problems, etc.) and cause unexpected behavior.
🔎 Proposed fix
// Check if settings already exist
- _, err = w.Client().Setting.Get(ctx, StorydenPrimarySettingsKey)
- if err == nil {
+ existing, err := w.Client().Setting.Get(ctx, StorydenPrimarySettingsKey)
+ if err != nil && !ent.IsNotFound(err) {
+ return fmt.Errorf("check existing settings: %w", err)
+ }
+
+ if existing != nil {
// Update existing settings
_, err = w.Client().Setting.UpdateOneID(StorydenPrimarySettingsKey).You'll need to import the ent package's error checking utilities and handle the three cases: found, not found, and actual error.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, err = w.Client().Setting.Get(ctx, StorydenPrimarySettingsKey) | |
| if err == nil { | |
| // Update existing settings | |
| _, err = w.Client().Setting.UpdateOneID(StorydenPrimarySettingsKey). | |
| SetValue(string(settingsJSON)). | |
| SetUpdatedAt(time.Now()). | |
| Save(ctx) | |
| if err != nil { | |
| return fmt.Errorf("update settings: %w", err) | |
| } | |
| log.Printf("Updated Storyden settings with MyBB forum name: %s", data.Settings["bbname"]) | |
| } else { | |
| existing, err := w.Client().Setting.Get(ctx, StorydenPrimarySettingsKey) | |
| if err != nil && !ent.IsNotFound(err) { | |
| return fmt.Errorf("check existing settings: %w", err) | |
| } | |
| if existing != nil { | |
| // Update existing settings | |
| _, err = w.Client().Setting.UpdateOneID(StorydenPrimarySettingsKey). | |
| SetValue(string(settingsJSON)). | |
| SetUpdatedAt(time.Now()). | |
| Save(ctx) | |
| if err != nil { | |
| return fmt.Errorf("update settings: %w", err) | |
| } | |
| log.Printf("Updated Storyden settings with MyBB forum name: %s", data.Settings["bbname"]) | |
| } else { |
🤖 Prompt for AI Agents
In cmd/import-mybb/transform/settings.go around lines 38 to 49, the Get call
treats any error as "not found"; change it to explicitly handle three cases: if
err == nil proceed with the Update path; else if ent.IsNotFound(err) proceed
with Create; otherwise return the error (e.g., fmt.Errorf("get settings: %w",
err)). Add the ent import (entgo.io/ent) to use ent.IsNotFound and ensure the
error message is returned for non-not-found failures so real errors
(network/permission) are not masked.
| i, err = w.client.AccountRoles.Delete().Exec(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to delete AccountRoless: %w", err) | ||
| } | ||
| logger.Success(fmt.Sprintf("Deleted %d existing AccountRoless", i)) | ||
|
|
There was a problem hiding this comment.
Typos in log messages.
Several log messages have grammatical errors:
- Line 73, 75, 191: "AccountRoless" should be "AccountRoles"
- Line 85: "Categorys" should be "Categories"
🔎 Proposed fixes
- return fmt.Errorf("failed to delete AccountRoless: %w", err)
+ return fmt.Errorf("failed to delete AccountRoles: %w", err)
}
- logger.Success(fmt.Sprintf("Deleted %d existing AccountRoless", i))
+ logger.Success(fmt.Sprintf("Deleted %d existing AccountRoles", i))- return fmt.Errorf("failed to delete Categorys: %w", err)
+ return fmt.Errorf("failed to delete Categories: %w", err)- logger.Success(fmt.Sprintf("Imported %d AccountRoless", len(result)))
+ logger.Success(fmt.Sprintf("Imported %d AccountRoles", len(result)))Also applies to: 85-87, 191-191
🤖 Prompt for AI Agents
In cmd/import-mybb/writer/storyden.go around lines 71-76 (and also update
occurrences at 85-87 and ~191), fix typos in log/messages: replace
"AccountRoless" with "AccountRoles" on lines 73 and 75 (and any other instances
at ~191), and replace "Categorys" with "Categories" on lines 85-87; ensure
plural spelling is correct in both the error messages and success logs so the
messages read e.g. "failed to delete AccountRoles" and "Deleted %d existing
AccountRoles" (and similarly for Categories).
- bbcode conversion - ancient broken encoding conversion - mostly 1:1 mapping
Southclaws
force-pushed
the
import-mybb
branch
from
e108ca9 to
8235929
Compare
1 participant
In progress...