Add pagination to treasury app #622
Conversation
kongzii
commented
Jan 6, 2025
•
kongzii
commented
WalkthroughThis pull request introduces enhancements to message handling and pagination features within the prediction market agent. Key changes include the addition of a new function for displaying paginated messages in the NFT treasury game application, updates to existing functions to incorporate pagination controls, and modifications to database query methods to support pagination through the introduction of Changes
Sequence DiagramsequenceDiagram
participant User
participant App as NFT Treasury Game App
participant DB as Database
User->>App: Request Function Calls
App->>App: Calculate Pagination
App->>DB: Fetch Messages with Offset
DB-->>App: Return Paginated Messages
App->>User: Display Messages with Navigation
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
prediction_market_agent/db/sql_handler.py (1)
Line range hint
37-52: Consider keyset pagination for large datasets.Offset-based pagination can be expensive for large tables since the database must scan all previous rows before reaching the offset. Keyset pagination (using a unique sequential column for ordering) is often more performant and avoids skipping large numbers of rows.
prediction_market_agent/db/long_term_memory_table_handler.py (1)
Line range hint
51-68: Ensure consistent pagination and ordering logic.Adding the
offsetparameter enhances pagination flexibility. For very large values ofoffset, consider performance implications. Additionally, confirm that sorting logic remains stable to prevent inconsistent result sets across pages.prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py (2)
187-205: Improve page-navigation boundary checks.The pagination logic is straightforward. Consider guarding against potential negative page values (already partially done by disabling the button). Also ensure no unintended state changes if multiple threads or sessions access the app (though typically Streamlit sessions are isolated).
Line range hint
238-292: Use unified pagination approach for unprocessed messages.You fetch only 10 messages by default and then print a note if there are more. Consider using the same pagination strategy as in the main message logs for consistency and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py(4 hunks)prediction_market_agent/db/agent_communication.py(1 hunks)prediction_market_agent/db/long_term_memory_table_handler.py(2 hunks)prediction_market_agent/db/sql_handler.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-build-image
- GitHub Check: pytest-docker
- GitHub Check: pytest
- GitHub Check: mypy
🔇 Additional comments (4)
prediction_market_agent/db/agent_communication.py (1)
15-22: Clamp the optional parameter or handle edge cases.When
nis provided, consider clampingnto ensure it’s non-negative and does not exceed the total unseen messages. This avoids edge cases such as providing a negative value or an integer larger thancount_unseen_messages.prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py (3)
40-43: The import statements appear consistent with usage.The newly added imports for transaction-related functions correctly reflect their usage in the code below.
206-216: Pagination-based retrieval from the database is coherent.Calling
.search(offset, limit)to retrieve a slice of records is aligned with the new pagination strategy. All tags look consistent, and integrating it with the UI is performed neatly.
294-294: Run frequency increased to 10 seconds.Running this fragment every 10 seconds reduces the system’s update overhead. Ensure this interval balances responsiveness with performance for your production environment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py (3)
187-204: Add upper limit check for Next button.While the pagination implementation is solid, the Next button should be disabled when there are no more messages to show. This prevents users from navigating to empty pages.
Consider updating the Next button implementation:
- if st.button("Next page"): + total_messages = len(long_term_memory_table_handler(nft_agent.identifier).search()) + max_pages = (total_messages + messages_per_page - 1) // messages_per_page + if st.button("Next page", disabled=st.session_state.page_number >= max_pages - 1): st.session_state.page_number += 1
273-292: Enhance message display UX.While the implementation is efficient, consider adding more user control over message display.
Consider these improvements:
- show_n = 10 + show_n = st.slider("Messages to show", min_value=5, max_value=50, value=10) n_messages = fetch_count_unprocessed_transactions(nft_agent.wallet_address) - messages = fetch_unseen_transactions(nft_agent.wallet_address, n=show_n) + sort_order = st.selectbox("Sort by", ["Newest first", "Oldest first"]) + messages = fetch_unseen_transactions( + nft_agent.wallet_address, + n=show_n, + reverse=(sort_order == "Newest first") + )
206-206: Consider adding manual refresh option.While increasing the refresh interval to 10 seconds improves server performance, users might need more immediate updates.
Add a manual refresh button to complement the automatic updates:
@st.fragment(run_every=timedelta(seconds=10)) def show_function_calls_part_messages( nft_agent: type[DeployableAgentNFTGameAbstract], messages_per_page: int, page_number: int, ) -> None: + col1, col2 = st.columns([0.9, 0.1]) + with col2: + if st.button("🔄", help="Refresh now"): + st.rerun() + with st.spinner("Loading agent's actions..."):Also applies to: 238-238, 294-294
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: mypy
🔇 Additional comments (1)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py (1)
40-43: LGTM! Import changes align with pagination requirements.The addition of
fetch_count_unprocessed_transactionscomplements the existingfetch_unseen_transactionsimport, supporting the new pagination functionality.
| @st.fragment(run_every=timedelta(seconds=10)) | ||
| def show_function_calls_part_messages( | ||
| nft_agent: type[DeployableAgentNFTGameAbstract], | ||
| messages_per_page: int, | ||
| page_number: int, | ||
| ) -> None: | ||
| with st.spinner("Loading agent's actions..."): | ||
| calls = long_term_memory_table_handler(nft_agent.identifier).search() | ||
| calls = long_term_memory_table_handler(nft_agent.identifier).search( | ||
| offset=page_number * messages_per_page, | ||
| limit=messages_per_page, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for robustness.
The function should handle edge cases and potential database errors gracefully.
Consider adding error handling:
@st.fragment(run_every=timedelta(seconds=10))
def show_function_calls_part_messages(
nft_agent: type[DeployableAgentNFTGameAbstract],
messages_per_page: int,
page_number: int,
) -> None:
+ if page_number < 0 or messages_per_page <= 0:
+ st.error("Invalid pagination parameters")
+ return
+
with st.spinner("Loading agent's actions..."):
- calls = long_term_memory_table_handler(nft_agent.identifier).search(
- offset=page_number * messages_per_page,
- limit=messages_per_page,
- )
+ try:
+ calls = long_term_memory_table_handler(nft_agent.identifier).search(
+ offset=page_number * messages_per_page,
+ limit=messages_per_page,
+ )
+ except Exception as e:
+ st.error(f"Failed to load agent's actions: {str(e)}")
+ return📝 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.
| @st.fragment(run_every=timedelta(seconds=10)) | |
| def show_function_calls_part_messages( | |
| nft_agent: type[DeployableAgentNFTGameAbstract], | |
| messages_per_page: int, | |
| page_number: int, | |
| ) -> None: | |
| with st.spinner("Loading agent's actions..."): | |
| calls = long_term_memory_table_handler(nft_agent.identifier).search() | |
| calls = long_term_memory_table_handler(nft_agent.identifier).search( | |
| offset=page_number * messages_per_page, | |
| limit=messages_per_page, | |
| ) | |
| @st.fragment(run_every=timedelta(seconds=10)) | |
| def show_function_calls_part_messages( | |
| nft_agent: type[DeployableAgentNFTGameAbstract], | |
| messages_per_page: int, | |
| page_number: int, | |
| ) -> None: | |
| if page_number < 0 or messages_per_page <= 0: | |
| st.error("Invalid pagination parameters") | |
| return | |
| with st.spinner("Loading agent's actions..."): | |
| try: | |
| calls = long_term_memory_table_handler(nft_agent.identifier).search( | |
| offset=page_number * messages_per_page, | |
| limit=messages_per_page, | |
| ) | |
| except Exception as e: | |
| st.error(f"Failed to load agent's actions: {str(e)}") | |
| return |
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py
Outdated
Show resolved
Hide resolved
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py (1)
211-221:⚠️ Potential issueAdd error handling for robustness.
The function should handle edge cases and potential database errors gracefully.
Consider adding error handling:
@st.fragment(run_every=timedelta(seconds=10)) def show_function_calls_part_messages( nft_agent: type[DeployableAgentNFTGameAbstract], messages_per_page: int, page_number: int, ) -> None: + if page_number < 0 or messages_per_page <= 0: + st.error("Invalid pagination parameters") + return + with st.spinner("Loading agent's actions..."): - calls = long_term_memory_table_handler(nft_agent.identifier).search( - offset=page_number * messages_per_page, - limit=messages_per_page, - ) + try: + calls = long_term_memory_table_handler(nft_agent.identifier).search( + offset=page_number * messages_per_page, + limit=messages_per_page, + ) + except Exception as e: + st.error(f"Failed to load agent's actions: {str(e)}") + return
🧹 Nitpick comments (2)
prediction_market_agent/db/sql_handler.py (1)
37-37: Consider removing the zero check for offset.The zero check for the offset parameter is unnecessary since SQLAlchemy's
offset()method handles zero values correctly. This would simplify the code.- if offset: - query = query.offset(offset) + query = query.offset(offset)Also applies to: 51-52
prediction_market_agent/db/long_term_memory_table_handler.py (1)
78-80: Consider adding date range parameters to count method.The count method could be more flexible by accepting optional date range parameters, similar to the search method.
- def count(self) -> int: - query_filters = self._get_query_filters(None, None) + def count( + self, + from_: DatetimeUTC | None = None, + to_: DatetimeUTC | None = None, + ) -> int: + query_filters = self._get_query_filters(from_, to_)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py(4 hunks)prediction_market_agent/db/long_term_memory_table_handler.py(2 hunks)prediction_market_agent/db/sql_handler.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-build-image
- GitHub Check: pytest
- GitHub Check: pytest-docker
- GitHub Check: mypy
🔇 Additional comments (5)
prediction_market_agent/db/sql_handler.py (1)
58-66: LGTM! Clean implementation of the count method.The count method is well-implemented, reusing the query filter logic consistently.
prediction_market_agent/db/long_term_memory_table_handler.py (2)
48-58: LGTM! Well-structured query filter extraction.Good refactoring to extract the common filter logic into a private method. This improves code reusability and maintainability.
60-76: LGTM! Clear and well-documented search method.The search method is well-documented and correctly implements pagination using the offset parameter.
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py (2)
187-208: LGTM! Well-implemented pagination controls.Good implementation of pagination with:
- Correct button disable logic
- Clear page number display
- Proper session state management
278-296: LGTM! Good implementation of message count and display.The implementation correctly:
- Fetches a limited number of messages
- Shows the total count of unprocessed messages
- Displays a clear message when there are more unprocessed messages
