Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: unified logs handler #1031

Merged
merged 11 commits into from
Dec 21, 2024

Conversation

kga245
Copy link
Contributor

@kga245 kga245 commented Dec 19, 2024

Log Handler Restructuring and Feature Enhancements

Overview

This PR implements several improvements focused on:

  • Restructuring logging functionality into a shared module
  • Adding type safety improvements
  • Adding new report download options
  • Setting up FastAPI backend infrastructure
  • Improving development workflow

Changes

Backend

  • New FastAPI Server (backend/server/app.py)
    • Created new FastAPI application with CORS middleware
    • Set up basic logging infrastructure
    • Configured cross-origin request handling for frontend communication

Frontend

  • Enhanced Report Access (frontend/nextjs/components/ResearchBlocks/AccessReport.tsx)
    • Added proper TypeScript interfaces for better type safety
    • Improved error handling for missing report paths
    • Added JSON download option alongside existing PDF and DocX
    • Added URL path sanitization
    • Improved accessibility with proper React attributes

Testing & Infrastructure

  • Logs Handler Refactor (tests/gptr-logs-handler.py, tests/report-types.py)
    • Moved CustomLogsHandler to shared module (src.logs_handler)
    • Updated handler initialization to include query parameter
    • Reduced code duplication
    • Simplified test files

Development Workflow

  • Git Configuration (.gitignore)
    • Added exclusions for log files (logs/)
    • Added exclusions for backups (.orig) - developer preference

Benefits

  • ✨ Better code organization through shared modules
  • 🛡️ Enhanced type safety with TypeScript interfaces
  • 🚀 New JSON download feature for reports
  • 🏗️ Improved architecture with proper backend separation
  • 🧹 Cleaner development workflow
  • 🔒 Better security practices in frontend

UATs

  • UAT: As a user, perform a research query and wait for it to finish. Download the Log by clicking the new "Download Log" button.
  • UAT: Repeat the above step. Now check the new "logs" directory in the source to see two new files. One is the .log file and the other is the .json report. The .log file captures the top level requests while the .json file is more verbose. Both will iterate periodically with server refreshes. They are not designed to cut new log files for every query but they do timeout, as it were, and begin again based on server inactivity.
  • Note: if the log directory does not exist, one will be created at first runtime.

Manual Testing Checklist

  • Verify log file creation and format
  • Test all download buttons with various report types
  • Download Links
    • Test PDF viewer link
    • Test DocX download
    • Test JSON logs download
    • Verify security attributes (target="_blank", rel="noopener noreferrer")
  • Optional: Verify console warnings appear when report paths are missing (open browser dev tools > Console tab)e.g. "Warning: No docx path provided"
  • Optional: Test CORS configuration:
    • URL Construction
      • Verify getReportLink() correctly handles different path formats
      • Test URL sanitization (removing leading slashes)
      • Verify fallback behavior when paths are missing

Notes

CORS Testing (Future)

  • The new FastAPI backend setup includes CORS middleware, but since there are no active endpoints yet, CORS testing can be deferred until API endpoints are implemented
  • Current PR only includes the initial CORS configuration setup

- Add CustomLogsHandler for unified logging
- Implement JSON and text file logging
- Add comprehensive test coverage for logging functionality
- Update pytest configuration for async tests
- Update gitignore patterns for log files
@ElishaKay
Copy link
Collaborator

ElishaKay commented Dec 19, 2024

Welcome @kga245,
Wow, this looks awesome!
I'll have a look at the changes in the frontend and tests folders this weekend

@kga245
Copy link
Contributor Author

kga245 commented Dec 19, 2024 via email

This was cruff from a different branch I was working on. My bad.
I noticed that if you do a very long query the file names that get generated can be larger than the host permits. So I added some truncation that should work pretty universally to limit the file name to under <250 characters total including the important naming convention of the prefixes.
@assafelovic
Copy link
Owner

@kga245 this is huge! Will review as well, can you point me the to the loom recording?

@assafelovic
Copy link
Owner

@ElishaKay I reviewed and looks great. Would you mind giving it another run + local testing and approve?

@ElishaKay ElishaKay merged commit 9c55fce into assafelovic:master Dec 21, 2024
@kga245
Copy link
Contributor Author

kga245 commented Dec 22, 2024

Great news about the merge. Thanks for that. @assafelovic here's the loom: https://www.loom.com/share/873efd40fe944bf68cfc9bf8a19ccdb1?sid=bcb36490-569a-4cb8-b7b9-b48a789358af

@ElishaKay I noticed the edits you made. I have a slight change to add today. I will commit, test and submit another PR which will leave your changes in tact. Adding back the log button for detailed reports.

I am also working on documentation for the logs so users will know how to work with them.

Question: I notice the copy to markdown button doesn't actually copy markdown (seems to be plain text). Considering we have a download markdown button should we consider just dropping the copy markdown button altogether? If not, then I might work on a change to change the download buttons to a dropdown select instead so we can fit in more download types without cluttering the UI. I think a download log as CSV and download log as markdown might also be useful to some useers. LMK what you prefer.

@kga245 kga245 deleted the feature/unified-logs-handler branch January 3, 2025 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants