Fix: Initialize IDGenerator map and improve Markdown #5851
Conversation
Amitverma0509
had a problem deploying
to
action
GitHub Actions
Failure
|
Automated Quality Checks (from CONTRIBUTING minimum standards)
These checks are a best-effort automation and do not replace human review. |
WalkthroughIntroduces an exported Has([]byte) bool method on IDGenerator, adds ensureInit() to lazily initialize the internal used map, and updates Generate and Put to call ensureInit before accessing the map. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant IDGen as IDGenerator
participant Init as ensureInit()
participant Map as used map
Caller->>IDGen: Generate(value)
IDGen->>Init: ensureInit()
Init-->>IDGen: used initialized
IDGen->>Map: read/update entries
Map-->>IDGen: result
IDGen-->>Caller: id
Caller->>IDGen: Put(value)
IDGen->>Init: ensureInit()
Init-->>IDGen: used initialized
IDGen->>Map: insert value
IDGen-->>Caller: ok
Caller->>IDGen: Has(value)
IDGen->>Init: ensureInit()
Init-->>IDGen: used initialized
IDGen->>Map: lookup value
Map-->>IDGen: boolean
IDGen-->>Caller: exists?
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
Amitverma0509
had a problem deploying
to
action
GitHub Actions
Failure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/markdown/convert.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Use standard formatting (gofmt) and idiomatic Go style
Favor small, testable functions in Go
Keep exported Go APIs documented with Go-style comments
Maintain ≥80% coverage for non-data packages and ≥90% for data packages when adding new testable Go code
Files:
pkg/markdown/convert.go
{**/*.go,.github/scripts/**/*.js}
📄 CodeRabbit inference engine (AGENTS.md)
Use American English spelling in code comments
Files:
pkg/markdown/convert.go
🧬 Code graph analysis (1)
pkg/markdown/convert.go (1)
pkg/slug/generator.go (1)
Generate(10-14)
🪛 GitHub Actions: tests
pkg/markdown/convert.go
[error] 66-66: compile error: IDGenerator (type) is not an expression
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
pkg/markdown/convert.go (3)
45-50: LGTM! Good defensive initialization pattern.The
ensureInitmethod correctly prevents nil map panics by lazily initializing the internal map.
54-54: LGTM! Prevents nil map panic.Calling
ensureInit()before accessing the map is the correct approach.
60-60: LGTM! Prevents nil map panic.Calling
ensureInit()before accessing the map is the correct approach.
| // Has checks if the given ID is already used. | ||
| func (g *IDGenerator) Has(value []byte) bool { | ||
| (g *IDGenerator) | ||
| return g.used[util.BytesToReadOnlyString(value)] | ||
| } |
There was a problem hiding this comment.
Critical: Fix compile error and add missing initialization.
Two issues must be fixed:
- Line 66 contains invalid syntax
(g *IDGenerator)causing a compile error. - The method accesses
g.usedwithout callingensureInit(), risking a nil map panic.
Apply this diff to fix both issues:
// Has checks if the given ID is already used.
func (g *IDGenerator) Has(value []byte) bool {
- (g *IDGenerator)
+ g.ensureInit()
return g.used[util.BytesToReadOnlyString(value)]
}📝 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.
| // Has checks if the given ID is already used. | |
| func (g *IDGenerator) Has(value []byte) bool { | |
| (g *IDGenerator) | |
| return g.used[util.BytesToReadOnlyString(value)] | |
| } | |
| // Has checks if the given ID is already used. | |
| func (g *IDGenerator) Has(value []byte) bool { | |
| g.ensureInit() | |
| return g.used[util.BytesToReadOnlyString(value)] | |
| } |
🧰 Tools
🪛 GitHub Actions: tests
[error] 66-66: compile error: IDGenerator (type) is not an expression
🤖 Prompt for AI Agents
In pkg/markdown/convert.go around lines 64 to 68, the Has method contains a
stray invalid token "(g *IDGenerator)" causing a compile error and it reads
g.used without ensuring initialization; remove the stray token and call
g.ensureInit() before accessing g.used, then return the map lookup result (e.g.,
ensure initialization, then return g.used[util.BytesToReadOnlyString(value)]).
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify test coverage for the new Has method.
As per coding guidelines, ensure ≥80% coverage for non-data packages when adding new testable code. Please verify that unit tests exist for the new Has method, including edge cases (empty map, existing ID, non-existing ID).
Run the following script to check for existing tests:
🏁 Script executed:
#!/bin/bash
# Description: Check if tests exist for IDGenerator methods
# Search for test files related to markdown package
fd -e go -g '*_test.go' pkg/markdown
# Search for test functions testing IDGenerator
rg -n 'func.*Test.*IDGenerator' pkg/markdown
rg -n 'func.*Test.*Has' pkg/markdown
# Check overall test coverage (if tests exist)
if fd -e go -g '*_test.go' pkg/markdown | grep -q .; then
echo "Test files found. Run 'go test -cover ./pkg/markdown' to check coverage."
fiLength of output: 101
🏁 Script executed:
#!/bin/bash
# Inspect test file for IDGenerator and Has coverage
echo "---------- convert_test.go ----------"
sed -n '1,200p' pkg/markdown/convert_test.go
echo "---------- Searching for Has ----------"
rg -n 'Has' pkg/markdown/convert_test.go || echo "No occurrences of 'Has'"
echo "---------- Searching for IDGenerator ----------"
rg -n 'IDGenerator' pkg/markdown/convert_test.go || echo "No occurrences of 'IDGenerator'"Length of output: 1477
Add unit tests for IDGenerator.Has
No tests currently cover the new Has method. Add table-driven tests for edge cases (empty generator, existing ID, non-existing ID) to meet ≥80% coverage for this package.
🧰 Tools
🪛 GitHub Actions: tests
[error] 66-66: compile error: IDGenerator (type) is not an expression
🤖 Prompt for AI Agents
In pkg/markdown/convert.go around lines 64 to 68, add unit tests for
IDGenerator.Has in a new or existing test file (pkg/markdown/convert_test.go)
using table-driven tests that cover at least: empty generator (no IDs added),
existing ID (add an ID to the generator.used map using
util.BytesToReadOnlyString on the byte slice), and non-existing ID (check a
different byte slice); for each case create a subtest with clear name, construct
the IDGenerator state, call Has with the byte slice, and assert the returned
bool matches the expected value; run go test and adjust tests until package
coverage for pkg/markdown is ≥80%.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical runtime panic in the IDGenerator by ensuring proper map initialization and adds functionality to check for existing IDs. The changes prevent nil map assignment errors that could crash the Markdown processing.
- Fixed potential runtime panic by adding lazy initialization for the IDGenerator map
- Added Has() method to check for existing IDs in the generator
- Enhanced robustness of the Markdown-to-HTML conversion process
|
|
||
| // Has checks if the given ID is already used. | ||
| func (g *IDGenerator) Has(value []byte) bool { | ||
| (g *IDGenerator) |
There was a problem hiding this comment.
Invalid syntax - this appears to be a type cast without an expression. This line should be removed or replaced with g.ensureInit() if initialization is intended.
| (g *IDGenerator) |
|
|
||
| // Has checks if the given ID is already used. | ||
| func (g *IDGenerator) Has(value []byte) bool { | ||
| (g *IDGenerator) |
There was a problem hiding this comment.
The Has() method doesn't call ensureInit() before accessing g.used map, which could cause a nil pointer dereference if the map hasn't been initialized yet.
| (g *IDGenerator) | |
| g.ensureInit() |
|
Hi @Amitverma0509, the automated checks for this PR failed. Please review the build logs and fix the issues. Specifically, the |
2 participants
Sumary
Changes Made
1.Fixed nil map panic in IDGenerator
Added an internal ensureInit() method to safely initialize g.used before access.
Prevents runtime error:
Added Has() method to IDGenerator
Improved ToHTML() robustness
We want to ensure high quality of the packages. Make sure that you've checked the boxes below before sending a pull request.
Not every repository (project) will require every option, but most projects should. Check the Contribution Guidelines for details.
Please provide some links to your package to ease the review
Pull Request content
Category quality
Note that new categories can be added only when there are 3 packages or more.
Packages added a long time ago might not meet the current guidelines anymore. It would be very helpful if you could check 3-5 packages above and below your submission to ensure that they also still meet the Quality Standards.
Please delete one of the following lines:
Thanks for your PR, you're awesome! 😎
Summary by CodeRabbit
Bug Fixes
Chores