-
Notifications
You must be signed in to change notification settings - Fork 0
Enables first article author to be subject owner #64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
* Article Viewing (Main Page): /article/{username}/{subjectname}
* File Preview (Diff Preview): /article/{username}/{subjectname}/_preview/{branch}/{filepath}
* Fork to Edit: /article/{username}/{subjectname}/_fork/{filepath}
* Edit File: /article/{username}/{subjectname}/_edit/{filepath}
* Create New File: /article/{username}/{subjectname}/_new/{filepath}
* Delete File: /article/{username}/{subjectname}/_delete/{filepath}
* Upload File: /article/{username}/{subjectname}/_upload/{filepath}
* Apply Diff/Patch: /article/{username}/{subjectname}/_diffpatch/{filepath}
* Cherry-Pick Commit: /article/{username}/{subjectname}/_cherrypick/{sha}/{filepath}
* Upload File to Server (Temporary): /article/{username}/{subjectname}/upload-file
* Remove Uploaded File from Server (Temporary): /article/{username}/{subjectname}/upload-remove
Co-authored-by: Pedro Gaudêncio <[email protected]>
- Fix incorrect closing tag </br> to <br> in new_repo_helper - Fix incorrect closing tag </di> to </div> in template_requirements - Fix missing closing tag for <i> element in template_requirements - Add missing semicolons in CSS declarations for consistency These typos were causing malformed HTML rendering in the repository creation form.
Add class="plus-symbol" to the SVG group element containing the plus symbol lines to enable the hover and focus styles defined in CSS. Without this class, the interactive styles were not being applied.
Fix inconsistent indentation of the repo-editor-header closing div tag to properly align with its opening tag. Also remove trailing tab from the opening tag line.
Change mode parameter from required to optional (mode?: string) to accurately reflect that it can be undefined based on the conditional check in the function body.
Use optional chaining for elCreateRepoErrorMessage to safely handle cases where the element might not exist. This prevents potential runtime errors when accessing textContent.
Make both comparisons in the first article check case-insensitive to ensure consistent behavior regardless of how the filename is capitalized in the tree path.
Add empty string checks after GetSubject calls to ensure the redirect URL is well-formed even if GetSubject fails or returns an empty string. Falls back to repository name if subject is empty.
Clarify that the function is used for both repository and article-based file operations, not just repositories.
Pass the repository's default branch from the template through FishboneGraph to CreateFirstArticleBubble component. This ensures the create article link uses the correct branch name that matches the repository configuration instead of assuming 'main'. Fallback to 'main' if defaultBranch is not provided.
Remove autofocus attribute from the hidden repo_name input and add it to the visible subject input field. This improves UX by focusing on the field users actually interact with and avoids issues with screen readers and browser focus logic. Co-authored-by: Pedro Gaudêncio <[email protected]>
Read the data-default-branch attribute from the DOM element and pass it as a prop when initializing the FishboneGraph component. This completes the fix for using the repository's actual default branch instead of hardcoding 'main'.
Add sanitization of branch name extracted from URL path for empty repositories using git.SanitizeRefPattern. This prevents potential issues if the branch name contains invalid git reference characters or is used in raw contexts.
- Remove redundant nested container div (tw-w-[32rem]) - Remove 'required' attribute from hidden repo_name field - Add scrollable wrapper for long template_requirements text - Fix div nesting by removing extra closing div and br tags - Improve button container spacing with tw-mt-8 These changes fix layout misalignment and ensure proper form structure without redundant containers. Co-authored-by: Pedro Gaudêncio <[email protected]>
* verify permission checks for article route group * test unauthenticated, non-owner, and owner access scenarios * confirm article routes behave identically to standard repo routes * validate "fork and edit" workflow functions correctly
* add batch count loading
* automatically fork from root if subject already has a root repository
* remove global subject uniqueness validation from form and model layer * subject uniqueness is handled by first-article-becomes-root logic
* add signedInUser prop to CreateFirstArticleBubble component * redirect non-owners to /repo/create instead of owner's editor URL * pass signed-in user info
* prioritize subject root repository over fork chain traversal * GetSubjectRootRepository finds first non-empty, non-fork repo for subject * ensures all users see the same global fork tree for a subject * falls back to fork chain traversal if no subject root exists
* add GetSubjectRootRepositoryExcluding to exclude current repo from search * update handleFirstArticleBecomesRoot to exclude current repository * fixes bug where root was determined by creation time, not submission time
* wrap check-and-update operations in db.WithTx() * pass transaction context to all database operations * return proper errors from transaction instead of just logging * prevents race condition where two users could both become roots
* improve BatchCountRepositoriesBySubjects function documentation * clarify behavior for non-existent subjects
* handle non-ErrRepoAlreadyExist errors within the loop * add found flag to track successful name discovery * add net/url import for proper URL escaping
* update instances of positional field initialization to named fields
* add maxRepoNameAttempts constant (100) with descriptive comment * replace hardcoded loop limit with the constant
…t create the fork
* verify IsFork, ForkID, and NumForks increment * verify calling on an already-forked repo is a no-op * verify attempting to fork to itself is a no-op * tests behavior with non-existent root (revealed a minor edge case issue)
* add checkForkTreeSizeLimit call for consistency with ForkRepository * add root repository existence validation before conversion * add tests for fork tree limit enforcement and root validation * prevents bypassing MaxForkTreeNodes via push-time conversion
* check if empty slice before NotIn
* refactor handleFirstArticleBecomesRoot to use ConvertNormalToForkRepository
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @pieer and @pedrogaudencio!
Opus 4.5 review ready:
Critical Issues
1. Duplicate Route Registration (Bug)
In routers/web/web.go, the same route group is registered twice:
// Article-based file operation routes - mirror the repository-based routes but use subject name
m.Group("/article/{username}/{subjectname}", func() {
registerRepoFileEditorRoutes(m, reqRepoCodeWriter)
}, reqSignIn, context.RepoAssignmentByOwnerAndSubject, reqUnitCodeReader)
// end "/article/{username}/{subjectname}": article-based file operations
// Article-based file operation routes - mirror the repository-based routes but use subject name
m.Group("/article/{username}/{subjectname}", func() {
registerRepoFileEditorRoutes(m, reqRepoCodeWriter)
}, reqSignIn, context.RepoAssignmentByOwnerAndSubject, reqUnitCodeReader)
// end "/article/{username}/{subjectname}": article-based file operationsFix: Remove the duplicate block entirely (lines 1219-1222 in the diff).
2. Context Cancellation in Goroutine (Bug)
In services/repository/push.go, the goroutine captures ctx which may be canceled after the HTTP request completes:
go func() {
c := cache.GetCache()
if c != nil {
// Trigger stats generation asynchronously (will be cached for future use)
_, _ = GetContributorStats(ctx, c, repo, repo.DefaultBranch)
}
}()Fix: Use a detached context:
go func() {
c := cache.GetCache()
if c != nil {
// Use background context since this runs after the request may complete
bgCtx := context.Background()
_, _ = GetContributorStats(bgCtx, c, repo, repo.DefaultBranch)
}
}()Potential Bugs
3. Go Version Compatibility Issue
In models/repo/subject.go, strings.FieldsSeq is used which is only available in Go 1.24+:
words := strings.FieldsSeq(subjectName)
for word := range words {
if strings.HasPrefix(word, keyword) {
return 2
}
}Fix: If targeting earlier Go versions, use strings.Fields instead:
words := strings.Fields(subjectName)
for _, word := range words {
if strings.HasPrefix(word, keyword) {
return 2
}
}4. Empty Repository Name Passed to Fork
In routers/web/repo/editor.go, getUniqueRepositoryName can return an empty string on database errors, which would cause a confusing error downstream:
forkedRepo := ForkRepoTo(ctx, ctx.Doer, repo_service.ForkRepoOptions{
BaseRepo: baseRepo,
Name: getUniqueRepositoryName(ctx, ctx.Doer.ID, baseRepo.Name), // Can be ""
Description: baseRepo.Description,
SingleBranch: baseRepo.DefaultBranch,
})Fix: Check the return value before proceeding:
repoName := getUniqueRepositoryName(ctx, ctx.Doer.ID, baseRepo.Name)
if repoName == "" {
ctx.ServerError("getUniqueRepositoryName", errors.New("failed to generate unique repository name"))
return
}
forkedRepo := ForkRepoTo(ctx, ctx.Doer, repo_service.ForkRepoOptions{
BaseRepo: baseRepo,
Name: repoName,
Description: baseRepo.Description,
SingleBranch: baseRepo.DefaultBranch,
})5. Missing Null Check in Template
In custom/templates/shared/subject/list.tmpl (and the duplicate templates/shared/subject/list.tmpl), there's a missing case when searching yields neither exact match nor similar results but user hasn't searched:
{{/* No results message */}}
{{if and (not .ExactMatch) (not .SimilarSubjects)}}
<div>
{{ctx.Locale.Tr "search.no_results"}}
</div>
{{end}}This shows "no results" even when there's a search keyword and the "create new subject" button is already shown. Consider:
{{/* No results message - only show if we don't already have the create button */}}
{{if and (not .ExactMatch) (not .SimilarSubjects) .ExactMatch}}
{{/* This condition can never be true - remove or fix logic */}}
{{end}}Fix: The no-results message is already handled by the create button section. Remove the redundant {{if and (not .ExactMatch) (not .SimilarSubjects)}} block at the end of the search keyword section since the create button serves as the "no exact match" indicator.
Code Improvements
6. Missing Import for errors Package
In the fix for issue #4 above, you'll need to add:
import "errors"Or use fmt.Errorf:
ctx.ServerError("getUniqueRepositoryName", fmt.Errorf("failed to generate unique repository name"))7. Inefficient Loop in Similarity Scoring
In models/repo/subject.go, the similarity scoring function iterates through results that were already filtered by the database:
func FindSimilarSubjects(ctx context.Context, keyword string, limit int, excludeIDs []int64) ([]*Subject, error) {
// ...
subjects := make([]*Subject, 0, limit)
sess := db.GetEngine(ctx).
Where("LOWER(name) LIKE ? AND LOWER(name) != ?", "%"+keyword+"%", keyword)
// ...
err := sess.OrderBy("updated_unix DESC").
Limit(limit).
Find(&subjects)The Limit(limit) is applied before scoring, so you might miss better matches. Consider fetching more results for scoring:
err := sess.OrderBy("updated_unix DESC").
Limit(limit * 2). // Fetch more for better scoring
Find(&subjects)
// ... then trim to limit after sorting by score8. Race Condition Note in Tests
The concurrent test TestFirstArticleBecomesRoot_ConcurrentEmptyCreation is testing concurrent creation of empty repos. The comment says:
// All 3 should succeed and none should be forks (because all are empty)
assert.Equal(t, 3, successCount, "Expected all 3 repositories to be created successfully")This is correct behavior but consider adding a test for concurrent creation where one becomes non-empty during the race to ensure the transaction isolation works correctly.
Template Duplication Note
Both custom/templates/shared/subject/list.tmpl and templates/shared/subject/list.tmpl contain the same changes. If custom/templates is meant to override templates, you may only need changes in one location. Verify this is intentional.
Summary
| Severity | Issue | Location |
|---|---|---|
| 🔴 Critical | Duplicate route registration | routers/web/web.go |
| 🔴 Critical | Context cancellation in goroutine | services/repository/push.go |
| 🟡 Medium | Go 1.24+ requirement (FieldsSeq) |
models/repo/subject.go |
| 🟡 Medium | Empty repo name not validated | routers/web/repo/editor.go |
| 🟢 Low | Template duplication | shared/subject/list.tmpl |
The custom template at custom/templates/shared/subject/list.tmpl should be the only file with the enhanced search functionality, following Forkana's template override pattern.
1. Remove duplicate route registration in web.go (lines 1229-1233) 2. Use context.Background() in goroutine in push.go to avoid context cancellation issues after HTTP request completes 4. Add check for empty repository name from getUniqueRepositoryName in editor.go before proceeding with fork 6. Use fmt.Errorf for error message (fmt already imported) 7. Fetch limit*2 results in FindSimilarSubjects before scoring, then trim to limit after sorting to avoid missing better matches 8. Add TestFirstArticleBecomesRoot_ConcurrentWithContentRace test to verify transaction isolation during concurrent repo creation with race condition where one becomes non-empty
|
Note on opus review:
|
1. Remove duplicate route registration in web.go (lines 1229-1233) 2. Use context.Background() in goroutine in push.go to avoid context cancellation issues after HTTP request completes 4. Add check for empty repository name from getUniqueRepositoryName in editor.go before proceeding with fork 6. Use fmt.Errorf for error message (fmt already imported) 7. Fetch limit*2 results in FindSimilarSubjects before scoring, then trim to limit after sorting to avoid missing better matches 8. Add TestFirstArticleBecomesRoot_ConcurrentWithContentRace test to verify transaction isolation during concurrent repo creation with race condition where one becomes non-empty
Fixes #61 and #70.