HTTP request logs screen by vincendep · Pull Request #1442 · lichess-org/mobile

vincendep · 2025-02-14T16:49:45Z

Specs:

Color in red response with code > 300.
Add a button to clear the logs
Use ListView with pagination (100 items per page?)

Implemetation:

should not reuse the existing http logger which was added for debugging locally during development
should wrap the default client returned in httpClientFactory to intercept all requests
save records in sqlite with a short retention period (7 days)
probably wise to save records by batches (like every 10?);
use app lifecycle events to save a pending incomplete batch when app is about to be exited (ie. put in background)

This PR closes #699

vincendep · 2025-02-19T15:23:48Z

Hi @veloce, could I've some feedback if I am on track or not? Following, I'll list what is currently missing in this PR:

Fix failing tests
Add Widget test for HttpLogScreen
Implement batch support in HttpLogStorage
Flush batch in AppLifecycle events
Improve throttling/debounce fetching logic
Polish UI: add empty screen, initial screen etc...
Improve Accessibility

veloce

Thanks for tackling this!

You're definitely on the right track. UI looks neat! (only feedback is that the status code is not aligned with the url).

Haven't looked in detail to the code, but I'm not sure why you want to improve the throttling logic to get the logs? Seems not really necessary but I may miss sth.

In the log screen, I'd add a confirm dialog when tapping the delete button.

lib/src/network/http.dart

vincendep · 2025-02-24T11:44:28Z

Hi @veloce. Regarding the throttling logic, my previous implementation was buggy but now seems to be fixed. I also added a confirmation dialog to delete the logs, then are missing only batching and internationalisation.

veloce · 2025-02-26T10:20:39Z

Hi @veloce. Regarding the throttling logic, my previous implementation was buggy but now seems to be fixed. I also added a confirmation dialog to delete the logs, then are missing only batching and internationalisation.

Thanks! will review asap.

Batching could be done in another PR I suppose (if we ever need it).

veloce

Nice work!

Internationalisation is not needed yet for this feature. We can leave the hardcoded English strings.

A widget test that test the whole log screen would be nice. Nothing fancy, but perhaps something like a test that loads the whole app (you can find example in app_test.dart), then navigates to the log screen and see the startup requests. That would test the whole logging logic at once and having a test that shows the number of requests made during app startup is not bad as well.
No need to test pagination or deletion.

lib/src/model/http_log/http_log_storage.dart

veloce · 2025-02-26T10:28:32Z

lib/src/model/http_log/http_log_storage.dart

+    }, conflictAlgorithm: ConflictAlgorithm.replace);
+  }
+
+  Future<void> update(


I don't think we need this, do we?

At the moment http logs are saved when a request is made and then updated with the status code when the response is received.

If you would like to use the save method in an upsert manner I have to understand how to manage primary key column that is used as a cursor for the pagination logic

No I just thought the log would never need to be updated.

Yeah maybe was better to save the log directly when receiving the response...only difference is that we cannot save unanswered http requests.

lib/src/model/http_log/http_log_controller.dart

vincendep · 2025-02-26T12:13:07Z

Sure I'll add some tests 👌🏻

veloce · 2025-02-28T08:54:04Z

lib/src/db/database.dart

        _createCorrespondenceGameTableV1(batch);
        _createChatReadMessagesTableV1(batch);
        _createGameTableV2(batch);
+        _createHttpLogTableV1(batch);


It should be _createHttpLogTableV4 imo (as in created in version 4).

veloce · 2025-02-28T08:56:37Z

lib/src/network/http.dart

-  Client call() {
+  const HttpClientFactory({this.onCreate});
+
+  final Client Function(Client client)? onCreate;


Would be better named wrapClient or wrapper.

veloce · 2025-03-11T11:00:49Z

@vincendep do you wish to continue working on this PR? there are a couple of renaming, and a test missing still.

vincendep · 2025-03-11T11:03:47Z

Hi @veloce, I would like to continue but at the moment I don't know when I will be able to do it.

veloce · 2025-03-11T11:15:40Z

Since it's almost done, if you prefer, I can take it over. I can add the test and merge this.

vincendep · 2025-03-11T11:17:28Z

No problem at all, as you wish.

vincendep marked this pull request as draft February 14, 2025 16:50

vincendep force-pushed the feat/http-request-logs branch from 1d6c4f3 to d184926 Compare February 18, 2025 15:03

veloce reviewed Feb 20, 2025

View reviewed changes

lib/src/network/http.dart Outdated Show resolved Hide resolved

vincendep marked this pull request as ready for review February 22, 2025 21:56

vincendep force-pushed the feat/http-request-logs branch from 2e3f4fb to 8b62bdb Compare February 24, 2025 12:34

veloce reviewed Feb 26, 2025

View reviewed changes

veloce reviewed Feb 28, 2025

View reviewed changes

vincendep added 19 commits March 4, 2025 13:39

store http logs in database

e48b98e

use explicit ids when saving http logs in database

4c1529a

wip: http log screen

4dbbf4d

run dart format

5483ec3

remove unused imports

fd3c98c

fix typo in database creation

c899988

use consistent naming convention for id field

3e03658

do not consume response stream

9c018b3

wip

5bb8526

http log controller provider

e3d0a41

run dart format

cb83b3c

refactor

9f258cd

refactor: rename LoggingClient to _RegisterCallbackClient

726d115

refactor

0c27b08

refactor: move callback registration in httpClientFactoryProvider

ee9e891

ignore unused parameter

f7a6874

run dart format

1f780e3

run dart format

bcbb887

remove unused import

956efde

vincendep added 2 commits March 4, 2025 13:39

refactor: rename classes

5275913

run dart format

156faf9

vincendep force-pushed the feat/http-request-logs branch from 7bb35aa to 156faf9 Compare March 4, 2025 12:39

veloce merged commit 2cafeab into lichess-org:main Mar 24, 2025
1 check passed

vincendep deleted the feat/http-request-logs branch April 14, 2025 07:40

Uh oh!

Conversation

vincendep commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vincendep commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

veloce left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vincendep commented Feb 24, 2025

Uh oh!

veloce commented Feb 26, 2025

Uh oh!

veloce left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

veloce Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

vincendep Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

vincendep Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

veloce Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

vincendep Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vincendep commented Feb 26, 2025

Uh oh!

veloce Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

veloce Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

veloce commented Mar 11, 2025

Uh oh!

vincendep commented Mar 11, 2025

Uh oh!

veloce commented Mar 11, 2025

Uh oh!

vincendep commented Mar 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vincendep commented Feb 14, 2025 •

edited

Loading

vincendep commented Feb 19, 2025 •

edited

Loading

veloce left a comment •

edited

Loading