Enables PDF bookmark generation #22
Open
+950
−5
Conversation
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
📝 WalkthroughWalkthroughThis PR adds PDF bookmark generation functionality to the ingest pipeline. It introduces a new IPdfBookmarkService interface with PdfBookmarkService and NoopPdfBookmarkService implementations, adds UsePdfBookmarks configuration flags, wires the service via dependency injection, integrates it into PdfRemediationProcessor, and includes integration tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In `@server.core/Remediate/Bookmarks/IPdfBookmarkService.cs`:
- Around line 7-12: The XML doc on IPdfBookmarkService currently says
implementations "must not throw", which is misleading because cancellation
should be allowed; update the <remarks> for the IPdfBookmarkService contract to
state that implementations must swallow and not propagate non-cancellation
exceptions but may allow OperationCanceledException (or other cancellation
exceptions) to propagate so callers can handle cancellations—mention that
implementers should catch Exception and rethrow only if it's a cancellation
(e.g., OperationCanceledException).
🧹 Nitpick comments (1)
tests/server.tests/Integration/Remediate/PdfBookmarkServiceTests.cs (1)
178-192: Consider centralizing repo-root discovery for reuse.This helper appears in multiple integration tests; a shared utility could reduce duplication and drift.
Comment on lines
+7
to
+12
| /// <summary> | ||
| /// Ensures the provided PDF has bookmarks (outlines) when possible. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Implementations must not throw; failures should be treated as a no-op. | ||
| /// </remarks> |
There was a problem hiding this comment.
Clarify the “must not throw” contract to allow cancellation.
Both the no-op and real implementation will throw OperationCanceledException on cancellation. The remark currently forbids all throws, which is misleading for callers/implementers. Consider tightening the doc to “swallow non-cancellation exceptions.”
✏️ Proposed doc fix
- /// <remarks>
- /// Implementations must not throw; failures should be treated as a no-op.
- /// </remarks>
+ /// <remarks>
+ /// Implementations should swallow non-cancellation exceptions and treat failures as a no-op.
+ /// <see cref="OperationCanceledException" /> may propagate when cancellation is requested.
+ /// </remarks>📝 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.
Suggested change
| /// <summary> | |
| /// Ensures the provided PDF has bookmarks (outlines) when possible. | |
| /// </summary> | |
| /// <remarks> | |
| /// Implementations must not throw; failures should be treated as a no-op. | |
| /// </remarks> | |
| /// <summary> | |
| /// Ensures the provided PDF has bookmarks (outlines) when possible. | |
| /// </summary> | |
| /// <remarks> | |
| /// Implementations should swallow non-cancellation exceptions and treat failures as a no-op. | |
| /// <see cref="OperationCanceledException" /> may propagate when cancellation is requested. | |
| /// </remarks> |
🤖 Prompt for AI Agents
In `@server.core/Remediate/Bookmarks/IPdfBookmarkService.cs` around lines 7 - 12,
The XML doc on IPdfBookmarkService currently says implementations "must not
throw", which is misleading because cancellation should be allowed; update the
<remarks> for the IPdfBookmarkService contract to state that implementations
must swallow and not propagate non-cancellation exceptions but may allow
OperationCanceledException (or other cancellation exceptions) to propagate so
callers can handle cancellations—mention that implementers should catch
Exception and rethrow only if it's a cancellation (e.g.,
OperationCanceledException).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Adds the ability to generate PDF bookmarks (outlines) for tagged PDFs. This feature is controlled by a configuration option and defaults to off.
IPdfBookmarkServiceto handle bookmark generation.PdfBookmarkServicethat extracts headings from tagged PDFs to create bookmarks.Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.