feat: 18-reading sequence#22
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a complete graph-based reading-sequence system: DB schema, graph construction from SQLite, PageRank + configurable betweenness centrality, SCC-condensed prioritized topological sequencing, two-pass file/function merging into ranked reading entries, persistence, CLI entry, and test/build wiring. ChangesReading Sequence Recommendation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 7
🧹 Nitpick comments (3)
CMakeLists.txt (1)
48-55: ⚡ Quick winGate GoogleTest and
AlgoTestsbehindBUILD_TESTING.Lines 48–55 and 207–218 unconditionally fetch and build test dependencies and targets. Production builds should not pull or compile test-only artifacts.
♻️ Suggested CMake adjustment
-FetchContent_Declare( - googletest - GIT_REPOSITORY https://github.com/google/googletest.git - GIT_TAG v1.15.2 -) - -FetchContent_MakeAvailable(tree-sitter SDL3 imgui imnodes googletest) +FetchContent_MakeAvailable(tree-sitter SDL3 imgui imnodes) + +if(BUILD_TESTING) + FetchContent_Declare( + googletest + GIT_REPOSITORY https://github.com/google/googletest.git + GIT_TAG v1.15.2 + ) + FetchContent_MakeAvailable(googletest) +endif() @@ -enable_testing() - -add_executable(AlgoTests - tests/algo/test_reading_sequencer.cpp - tests/algo/test_pagerank.cpp - tests/algo/test_score_combiner.cpp - tests/algo/test_betweenness_centrality.cpp - tests/algo/test_algo_merge.cpp -) -target_include_directories(AlgoTests PRIVATE src) -target_link_libraries(AlgoTests PRIVATE algo GTest::gtest_main) -add_test(NAME AlgoTests COMMAND AlgoTests) +if(BUILD_TESTING) + enable_testing() + + add_executable(AlgoTests + tests/algo/test_reading_sequencer.cpp + tests/algo/test_pagerank.cpp + tests/algo/test_score_combiner.cpp + tests/algo/test_betweenness_centrality.cpp + tests/algo/test_algo_merge.cpp + ) + target_include_directories(AlgoTests PRIVATE src) + target_link_libraries(AlgoTests PRIVATE algo GTest::gtest_main) + add_test(NAME AlgoTests COMMAND AlgoTests) +endif()🤖 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 `@CMakeLists.txt` around lines 48 - 55, The CMake fetch and build of googletest and the test target AlgoTests are unconditional; gate these behind CMake's BUILD_TESTING option by wrapping the FetchContent_Declare(FetchContent_MakeAvailable(googletest)) and the AlgoTests target/add_subdirectory logic in an if(BUILD_TESTING) ... endif() block, ensure you call include(CTest) or enable_testing() earlier so BUILD_TESTING is defined, and only add_link_libraries or add_test entries for AlgoTests when BUILD_TESTING is ON.src/algo/AlgoRunner.cpp (1)
148-148: 💤 Low valueReturn value from
AlgoDbWriter::write()is discarded.If persistence fails, the caller receives valid in-memory results without knowing the DB write failed. Consider propagating the error or logging a warning.
♻️ Suggested improvement
- AlgoDbWriter::write(dbPath, merged, cfg); + int write_rc = AlgoDbWriter::write(dbPath, merged, cfg); + if (write_rc != SQLITE_OK) { + std::fprintf(stderr, "AlgoRunner: DB write failed with code %d\n", write_rc); + } return merged;🤖 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 `@src/algo/AlgoRunner.cpp` at line 148, The call to AlgoDbWriter::write(dbPath, merged, cfg) in AlgoRunner.cpp currently drops its return value; update the caller to check the returned status/result from AlgoDbWriter::write and handle failures by either returning an error up the call chain or at minimum logging a warning/error via the existing logging mechanism; specifically, detect a non-success return from AlgoDbWriter::write and then propagate that error from the surrounding function (or convert it into an appropriate exception/Status) or call the logger to record the DB write failure along with context (dbPath, cfg or merged identifier) so callers are aware the persistence step failed.src/algo/ReadingSequencer.cpp (1)
56-63: ⚡ Quick winAdd a deterministic tie-breaker for equal SCC scores.
When
super_score[a] == super_score[b], pop order is left to heap internals. Adding a secondary key (e.g., SCC index) keeps output stable across environments.Suggested fix
auto cmp_super = [&](int a, int b) { - return super_score[a] < super_score[b]; // max-heap + if (super_score[a] != super_score[b]) return super_score[a] < super_score[b]; + return a > b; // smaller SCC index first for deterministic ordering };🤖 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 `@src/algo/ReadingSequencer.cpp` around lines 56 - 63, The comparator cmp_super and priority_queue pq use only super_score, which leaves ties non-deterministic; change cmp_super to break ties deterministically by comparing the SCC index (e.g., if super_score[a] != super_score[b] return super_score[a] < super_score[b]; else return a > b to keep the highest-index popped last) so that pq ordering is stable; update the lambda used to construct pq (cmp_super) and ensure the same tie-break logic is used anywhere pq.push/pop is relied on (references: cmp_super, super_score, pq, S, indegree).
🤖 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 `@src/algo_main.cpp`:
- Around line 14-19: Wrap the calls to AlgoDbWriter::loadConfig(dbPath) and
AlgoRunner::run(dbPath, cfg) in a try/catch block that catches std::exception
(and a catch-all) to prevent abrupt termination; on error print a clear
diagnostic to stderr (including the exception.what() or a descriptive message)
via std::fprintf and return a non-zero exit code instead of 0. Ensure you still
print the successful "computed ... entries" message using result.entries.size()
when no exception occurs, and map different failure types to distinct non-zero
codes if useful (e.g., config load failure vs. run failure) while keeping the
main function returning controlled error codes.
In `@src/algo/AlgoDbWriter.cpp`:
- Around line 63-74: The upsert function ignores return codes from
sqlite3_prepare_v2 and sqlite3_step so DB errors are lost; change upsert to
return an int status, check and return the result of sqlite3_prepare_v2 and
sqlite3_step (and handle bind failures), call sqlite3_finalize in all paths, and
include sqlite3_errmsg(db) in any logged error; then update updateConfig to
check upsert's return value and propagate or log errors instead of always
returning SQLITE_OK.
In `@src/algo/BetweennessCentrality.cpp`:
- Around line 79-87: The code can divide by zero when samples == 0 (k <= 0);
modify the scaling block in BetweennessCentrality.cpp to guard against samples
== 0 before computing scale and multiplying bc. Specifically, using the existing
symbols (samples, N, k, bc, scale, order, brandes_source), add a check like "if
(samples == 0) return;" or skip the scaling loop when samples == 0 so you never
compute N / samples; ensure other logic (the brandes_source loop) remains
unchanged because it already won't run when samples == 0.
In `@src/algo/Graph.h`:
- Around line 3-7: The header defines NodeId as uint32_t but doesn't include
<cstdint>, relying on transitive includes; update src/algo/Graph.h to add an
explicit `#include` <cstdint> so uint32_t is defined in this header (i.e., ensure
the file that declares NodeId includes <cstdint> before using uint32_t).
In `@src/algo/GraphBuilder.cpp`:
- Around line 103-106: The code reads CALLS/INHERITS rows via
sqlite3_prepare_v2/stmt and inserts every returned (source_id, target_id) row,
causing parallel function-level edges; fix by deduplicating before insertion:
while iterating sqlite3_step results (after extracting source_id and target_id),
maintain a local unordered_set (or std::set) of seen pairs (e.g.,
pair<uint64_t,uint64_t> or a combined key string/uint64) and only perform the
existing insertion logic if the pair is not already in the set, skipping
duplicates so each function-level dependency is inserted once.
In `@src/algo/ReadingSequencer.cpp`:
- Around line 27-31: The code assumes combined_score has at least one element
per graph node when computing super_score and later uses combined_score[u]; add
an explicit guard before indexing: verify combined_score.size() >= g.size() (or
check u < combined_score.size() inside the loops over sccs) and handle the
failure path (e.g., throw a descriptive exception or return an error) to avoid
out-of-bounds access; update the loops that reference combined_score (the loop
computing super_score with symbols super_score, sccs, NodeId, S and the later
uses around lines 73–76) to perform this check before any combined_score[u]
read.
In `@src/algo/Scoring.cpp`:
- Around line 89-101: The code computes combined scores from pr and bc without
validating inputs: ensure pr.size() == bc.size() before proceeding and
return/throw if they differ, and check that total = alpha + beta is not zero
before dividing (handle zero by returning empty, using equal weights, or
signaling an error). Validate sizes at start using pr and bc, compute total =
alpha + beta and if total == 0 handle it, then compute w_pr = alpha/total and
w_bc = beta/total and call minmax_normalize(pr)/minmax_normalize(bc) and
populate score vector as before (referencing symbols pr, bc, alpha, beta, total,
w_pr, w_bc, minmax_normalize, score).
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 48-55: The CMake fetch and build of googletest and the test target
AlgoTests are unconditional; gate these behind CMake's BUILD_TESTING option by
wrapping the FetchContent_Declare(FetchContent_MakeAvailable(googletest)) and
the AlgoTests target/add_subdirectory logic in an if(BUILD_TESTING) ... endif()
block, ensure you call include(CTest) or enable_testing() earlier so
BUILD_TESTING is defined, and only add_link_libraries or add_test entries for
AlgoTests when BUILD_TESTING is ON.
In `@src/algo/AlgoRunner.cpp`:
- Line 148: The call to AlgoDbWriter::write(dbPath, merged, cfg) in
AlgoRunner.cpp currently drops its return value; update the caller to check the
returned status/result from AlgoDbWriter::write and handle failures by either
returning an error up the call chain or at minimum logging a warning/error via
the existing logging mechanism; specifically, detect a non-success return from
AlgoDbWriter::write and then propagate that error from the surrounding function
(or convert it into an appropriate exception/Status) or call the logger to
record the DB write failure along with context (dbPath, cfg or merged
identifier) so callers are aware the persistence step failed.
In `@src/algo/ReadingSequencer.cpp`:
- Around line 56-63: The comparator cmp_super and priority_queue pq use only
super_score, which leaves ties non-deterministic; change cmp_super to break ties
deterministically by comparing the SCC index (e.g., if super_score[a] !=
super_score[b] return super_score[a] < super_score[b]; else return a > b to keep
the highest-index popped last) so that pq ordering is stable; update the lambda
used to construct pq (cmp_super) and ensure the same tie-break logic is used
anywhere pq.push/pop is relied on (references: cmp_super, super_score, pq, S,
indegree).
🪄 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: 0a3cfaaa-b629-4fe5-b213-e432d44ebcae
📒 Files selected for processing (19)
CMakeLists.txtimgui.inisrc/algo/AlgoConfig.hsrc/algo/AlgoDbWriter.cppsrc/algo/AlgoDbWriter.hsrc/algo/AlgoRunner.cppsrc/algo/AlgoRunner.hsrc/algo/BetweennessCentrality.cppsrc/algo/BetweennessCentrality.hsrc/algo/Graph.cppsrc/algo/Graph.hsrc/algo/GraphBuilder.cppsrc/algo/GraphBuilder.hsrc/algo/ReadingSequencer.cppsrc/algo/ReadingSequencer.hsrc/algo/Scoring.cppsrc/algo/Scoring.hsrc/algo_main.cppsrc/db/db.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
읽는 순서 추천 알고리즘 구현. PageRank와 Betweeness Connectivity 알고리즘을 활용하였다.
Changes
+42 −1 CMakeLists.txt
+12 −0 imgui.ini
+33 −0 src/algo/AlgoConfig.h
+240 −0 src/algo/AlgoDbWriter.cpp
+23 −0 src/algo/AlgoDbWriter.h
+150 −0 src/algo/AlgoRunner.cpp
+77 −0 src/algo/AlgoRunner.h
+127 −0 src/algo/BetweennessCentrality.cpp
+31 −0 src/algo/BetweennessCentrality.h
+79 −0 src/algo/Graph.cpp
+25 −0 src/algo/Graph.h
+220 −0 src/algo/GraphBuilder.cpp
+32 −0 src/algo/GraphBuilder.h
+93 −0 src/algo/ReadingSequencer.cpp
+18 −0 src/algo/ReadingSequencer.h
+104 −0 src/algo/Scoring.cpp
+25 −0 src/algo/Scoring.h
+20 −0 src/algo_main.cpp
+23 −1 src/db/db.cpp
Related Issue
Closes #18
Checklist