Implement PageReader and DatabaseReader services with optimized API architecture #3
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
|
@MaestroError 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs. I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review. |
Co-authored-by: MaestroError <46760939+MaestroError@users.noreply.github.com>
Co-authored-by: MaestroError <46760939+MaestroError@users.noreply.github.com>
Co-authored-by: MaestroError <46760939+MaestroError@users.noreply.github.com>
Copilot
AI
changed the title
Copilot finished work on behalf of
MaestroError
September 8, 2025 17:31
|
@copilot Here are the new tasks for you: Rewrite contentManager to
|
…h new API Co-authored-by: MaestroError <46760939+MaestroError@users.noreply.github.com>
... I've successfully implemented all the requested changes in commit ✅ Completed TasksPageReader Service (replaces ContentManager):
DatabaseReader Service:
Updated Database Object:
Updated Page Object:
Updated PEST Tests:
The new architecture optimizes API calls and provides a cleaner, more consistent interface while maintaining all existing functionality. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot finished work on behalf of
MaestroError
September 9, 2025 07:55
MaestroError
requested changes
Sep 9, 2025
There was a problem hiding this comment.
@copilot Requested some changes, please solve them and assign PR back to me for review
src/Objects/Database.php
Outdated
Comment on lines
43
to
48
| $this->getChildPages()->each(function (Page $page) use ($pageReader) { | ||
| $pageWithContent = $pageReader->read($page->getId()); | ||
| $page->setContent($pageWithContent->getContent()); | ||
| $page->setChildPages($pageWithContent->getChildPages()); | ||
| $page->setChildDatabases($pageWithContent->getChildDatabases()); | ||
| }); |
There was a problem hiding this comment.
If it's possible, just return to the collection the page returned from pageReader
There was a problem hiding this comment.
Updated to replace the entire page object in the collection instead of updating properties individually. The method now uses map() to return the page objects from pageReader directly. (29ccf80)
src/Objects/Page.php
Outdated
Comment on lines
113
to
118
| $this->getChildPages()->each(function (Page $page) use ($pageReader) { | ||
| $pageWithContent = $pageReader->read($page->getId()); | ||
| $page->setContent($pageWithContent->getContent()); | ||
| $page->setChildPages($pageWithContent->getChildPages()); | ||
| $page->setChildDatabases($pageWithContent->getChildDatabases()); | ||
| }); |
There was a problem hiding this comment.
Just use the page returned from pageReader, since it returns the Page object, they are interchangable, no need to set different properties separately
There was a problem hiding this comment.
Changed to use the page object returned from pageReader directly instead of setting individual properties. Now using map() to replace page objects in the collection. (29ccf80)
src/Objects/Page.php
Outdated
Comment on lines
133
to
137
| $this->getChildDatabases()->each(function (Database $database) use ($databaseReader) { | ||
| $databaseWithContent = $databaseReader->read($database->getId()); | ||
| $database->setTableContent($databaseWithContent->getTableContent()); | ||
| $database->setChildPages($databaseWithContent->getChildPages()); | ||
| }); |
There was a problem hiding this comment.
The same here, read the database by ID of current one and swap current database with the returned from databaseReader
There was a problem hiding this comment.
Updated to replace the entire database object with the one returned from databaseReader instead of setting properties separately. Now using map() to swap the objects in the collection. (29ccf80)
…ties - Updated Database.readItemsContent() to replace page objects in collection - Updated Page.readChildPagesContent() to replace page objects in collection - Updated Page.readChildDatabasesContent() to replace database objects in collection - Methods now use map() to replace objects instead of each() to update properties Co-authored-by: MaestroError <46760939+MaestroError@users.noreply.github.com>
I've addressed all the requested changes in commit 29ccf80:
All methods now use Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot finished work on behalf of
MaestroError
September 9, 2025 08:26
|
Remove:
Also, update/remove tests to make sure no removed items are used there |
Removed all requested files and methods in a56d89f:
The DatabaseTable service now only contains the Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: MaestroError <46760939+MaestroError@users.noreply.github.com>
Copilot
AI
changed the title
Copilot finished work on behalf of
MaestroError
September 9, 2025 08:55
Task 1Add Task 2Create example script which fetches the page with Database tables ( Check toggle-fetch-example.php as example |
- Add recursive readAllPagesContent method to Page object with API warning - Add note in README.md about API limits for recursive reading - Create page-reader-example.php demonstrating PageReader with databases and child pages - Add comprehensive test for recursive page reading functionality Co-authored-by: MaestroError <46760939+MaestroError@users.noreply.github.com>
... I've completed both tasks successfully in commit c91da9c: Task 1 -
Task 2 - Example script:
The implementation includes comprehensive test coverage and all 109 tests are passing. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot finished work on behalf of
MaestroError
September 9, 2025 09:25
|
Please review all changes we made and:
|
Co-authored-by: MaestroError <46760939+MaestroError@users.noreply.github.com>
I've completed both requested tasks: Task 1 - Missing Test Coverage:
Task 2 - Comprehensive PR Description:
The PR is now fully documented and thoroughly tested. (3d4d554) Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot finished work on behalf of
MaestroError
September 9, 2025 13:06
MaestroError
approved these changes
Sep 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This PR implements a new optimized service architecture for fetching Notion page content and converting it to markdown with support for child pages and databases, replacing the deprecated ContentManager approach.
Key Features Implemented
PageReader Service (replaces ContentManager)
The new
PageReaderclass provides optimized functionality:getPageand builds initial Page objectgetBlockChildrenonly onceDatabaseReader Service
The new
DatabaseReaderclass handles database content processing:getDatabaseonce and builds initial Database objectqueryDataSourceonly onceUpdated Database Object
Modified the Database object API:
fetchAsTableandfetchItemsmethods (replaced by DatabaseReader)$itemsproperty (now uses$childPagesfrom HasChildPages trait)readItemsContentmethod using PageReader service for each itemUpdated Page Object
Modified the Page object API:
fetchContent,fetchChildDatabases&fetchChildPagesmethods (replaced by PageReader)readChildPagesContentmethod using PageReader service for each child pagereadChildDatabasesContentmethod using DatabaseReader service for each child databasereadAllPagesContentmethod for recursive page reading (with API limit warning)Enhanced DatabaseTable Service
Streamlined capabilities:
convertQueryToMarkdownTablemethod for processing query response data directlyfetchDatabaseAsMarkdownTable&fetchDatabaseItemsmethodsCleanup and Architecture Improvements
CONTENT_MANAGER.md,content-manager-example.php,src/Services/ContentManager.php)fetchDatabaseAsMarkdownTable,fetchDatabaseItems)Examples and Documentation
page-reader-example.phpdemonstrating complete PageReader usage with databases and child pagesreadAllPagesContentmethod with API limit warningsTechnical Implementation
Usage Examples
PageReader Service
DatabaseReader Service
Complete Example
See
page-reader-example.phpfor a comprehensive example that demonstrates:The implementation follows the existing codebase patterns while providing the optimized, single-call architecture specified in the requirements and removes all deprecated components for a cleaner, more maintainable codebase.
Fixes #2.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.