-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
feat: access client side logging from Test Tools Modal #38803
feat: access client side logging from Test Tools Modal #38803
Conversation
if (!shouldStoreLogs) { | ||
Console.setShouldStoreLogs(true); | ||
} else { | ||
if (capturedLogs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these could be simplified with early returns on all the edge cases (like no logs captured)
<Switch | ||
accessibilityLabel="Client side logging" | ||
isOn={!!shouldStoreLogs} | ||
onToggle={() => (shouldStoreLogs ? Console.disableLoggingAndFlushLogs() : Console.setShouldStoreLogs(true))} | ||
onToggle={onToggleClientSideLogging} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use <ClientSideLoggingToolMenu />
export default withOnyx<ClientSideLoggingToolProps, ClientSideLoggingToolMenuOnyxProps>({ | ||
capturedLogs: { | ||
key: ONYXKEYS.LOGS, | ||
}, | ||
shouldStoreLogs: { | ||
key: ONYXKEYS.SHOULD_STORE_LOGS, | ||
}, | ||
})(ClientSideLoggingToolMenu); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to subscribe to onyx keys twice. Only subscribe in BaseClientSideLoggingToolMenu
and pass the required values through the props. (or the other way around)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, now i am subscribing to Onyx only in ClientSideLogginToolMenu
and passing it down to base component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to recent change I see no use in subscribing in ClientSideLogginToolMenu
. Let's subscribe in the base component instead
const logs = Object.values(capturedLogs as Log[]); | ||
const logsWithParsedMessages = parseStringifyMessages(logs); | ||
localFileCreate('logs', JSON.stringify(logsWithParsedMessages, null, 2)).then((localFile) => { | ||
if (getOperatingSystem() === CONST.OS.ANDROID) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid platform specific code. Instead write a index.android.tsx
file
import type {OnyxEntry} from 'react-native-onyx'; | ||
import type {Log} from '@libs/Console'; | ||
|
||
type CapturedLogs = Record<number, Log>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In ONYXKEYS please use that type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change
[ONYXKEYS.LOGS]: Record<number, OnyxTypes.Log>;
to:
[ONYXKEYS.LOGS]: OnyxTypes.CapturedLogs;
Console.disableLoggingAndFlushLogs(); | ||
return; | ||
} | ||
const logs = Object.values(capturedLogs as Log[]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const logs = Object.values(capturedLogs as Log[]); | |
const logs = Object.values(capturedLogs); |
<Button | ||
small | ||
text="Share" | ||
onPress={onShareLogs} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass file
as a param in onShareLogs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or better remove the onShareLogs
prop and let the button always call Share.open
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has to be platform-specific, as there is no share option for web/desktop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's pass the file prop through onShareLogs for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually there is no benefit in doing this.
@s77rt thanks for your review, I have addressed all of your comments |
function ClientSideLoggingToolMenu({capturedLogs, shouldStoreLogs}: ClientSideLoggingToolProps) { | ||
const onToggle = (logs: Log[]) => { | ||
localFileDownload('logs', JSON.stringify(logs, null, 2)); | ||
Console.disableLoggingAndFlushLogs(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this line to BaseClientSideLoggingToolMenu.onToggle
src/libs/Console/index.ts
Outdated
* @param logs Logs captured on the current device | ||
* @returns CapturedLogs with parsed messages | ||
*/ | ||
const parseStringifyMessages = (logs: Log[]) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const parseStringifyMessages = (logs: Log[]) => { | |
function parseStringifyMessages(logs: Log[]): Log[] { |
export default withOnyx<ClientSideLoggingToolProps, ClientSideLoggingToolMenuOnyxProps>({ | ||
capturedLogs: { | ||
key: ONYXKEYS.LOGS, | ||
}, | ||
shouldStoreLogs: { | ||
key: ONYXKEYS.SHOULD_STORE_LOGS, | ||
}, | ||
})(ClientSideLoggingToolMenu); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to recent change I see no use in subscribing in ClientSideLogginToolMenu
. Let's subscribe in the base component instead
import type {OnyxEntry} from 'react-native-onyx'; | ||
import type {Log} from '@libs/Console'; | ||
|
||
type CapturedLogs = Record<number, Log>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change
[ONYXKEYS.LOGS]: Record<number, OnyxTypes.Log>;
to:
[ONYXKEYS.LOGS]: OnyxTypes.CapturedLogs;
RNFetchBlob.MediaCollection.copyToMediaStore( | ||
{ | ||
name: localFile.newFileName, | ||
parentFolder: '', | ||
mimeType: 'text/plain', | ||
}, | ||
'Download', | ||
localFile.path, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to copy the files to the media store?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, on Android it is required to copy the file to media store to make it available in the public download directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @TMisiukiewicz was there a reason to first save the file in RNFetchBlob.fs.dirs.DocumentDir
and then copy in downloads, Can we save it directly in DownloadDir, is there a catch I am missing ?
<Button | ||
small | ||
text="Share" | ||
onPress={onShareLogs} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's pass the file prop through onShareLogs for now
Not sure I can test on Android. It does not seem to be working (file not being saved?) Screen.Recording.2024-03-25.at.3.03.12.PM.mov |
On toggle on I think we need to hide the previous path and share buttons Screen.Recording.2024-03-25.at.3.00.58.PM.mov |
Pressing Share causes the illustration to disappear Screen.Recording.2024-03-25.at.2.42.38.PM.mov |
Spacing of what exactly? Looks pretty good to me personally. |
@shawnborton The path (in muted text) and the Logs menu item |
Got it - yeah, I think it works. |
Who is this waiting on -- @s77rt ? what is the next step and when will it happen? |
@s77rt could you elaborate more on that? because I'm not sure I understand what exactly needs to be done, do you want to use both |
@TMisiukiewicz Can you please unify the testing steps? I think this is testable on all platforms |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.movAndroid: mWeb Chromemweb-chrome.moviOS: Nativeios.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
I just tested it by sending it with an email, seems like a file is visible as an attachment and once sent, it's possible to open it and read the content: Record_2024-03-26-12-24-14.mp4Record_2024-03-26-12-24-33.mp4I've updated test steps for Web & desktop for all platforms together with missing recordings for web & desktop based on Troubleshoot section |
@TMisiukiewicz Thank you! |
We did not find an internal engineer to review this PR, trying to assign a random engineer to #38456 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this feature improvement, and tests well! Just a few minor thoughts on the code
const styles = useThemeStyles(); | ||
return ( | ||
<> | ||
<TestToolRow title="Client side logging"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a translation for this phrase? I see that it's consistent with other titles in Tools (all are hardcoded English) so NAB, but curious if there's a reason for not translating
<TestToolRow title="Logs"> | ||
<Button | ||
small | ||
text="Share" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an existing translation for this, let's use it
<TestToolRow title="Profile trace"> | ||
<Button | ||
small | ||
text="Share" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above
src/libs/Console/index.ts
Outdated
* @param logs Logs captured on the current device | ||
* @returns CapturedLogs with parsed messages | ||
*/ | ||
function parseStringifyMessages(logs: Log[]): Log[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps Stringified
instead?
function parseStringifyMessages(logs: Log[]): Log[] { | |
function parseStringifiedMessages(logs: Log[]): Log[] { |
@amyevans thanks for you comments! I have updated the translations, I think it makes sense to use translation for Client side logging as well, would be good to remove all existing hard-coded phrases in the future |
Okay great! I've requested translations from our Spanish speakers and will update here once I've got them |
Here are the translations:
Let's get those in and then we should be set to 🚀 |
@amyevans thank you, updated! |
🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.60-13 🚀
|
Hi 👋 @TMisiukiewicz @s77rt ,For Client side logging, Why are we showing the share option for Android and iOS only? I am reviewing a PR that captures profiling data for web/desktop, and there we are showing the share button, but it downloads the file on the web. Moving forward with that PR, I need to understand why we hide the Share button for web/desktop for Client side logging. |
@jayeshmangwani On web there is no share functionality. The log file is downloaded automatically after you stop capturing the logs. |
Details
Fixed Issues
$ #38456
PROPOSAL:
Tests
iOS/Android:
Open dev menu and select "Open Test Preferences"
iOS Web/Android Web
Use physical device and use four-finger tap or switch
isVisible={!!isTestToolsModalOpen}
toisVisible
insrc/components/TestToolsModal.tsx
iOS/Android
3. Verify path of the file is visible
4. Press "Share" and verify sharing modal appeared
5. Go to the Files on the device and verify it was saved
iOS Web/Android Web
3. Verify file was downloaded on the device
All platforms - access from Troubleshoot page
iOS/Android
4. Verify the filepath is displayed and Share button is visible
5. Press "Share" and verify the Share menu is visible
Other platforms
4. Verify file was downloaded to the device
Offline tests
QA Steps
iOS/Android
8. Verify path of the file is visible
9. Press "Share" and verify sharing modal appeared
10. Go to the Files on the device and verify it was saved
iOS Web/Android Web
6. Verify file was downloaded on the device
All platforms - access from Troubleshoot page
iOS/Android
4. Verify the filepath is displayed and Share button is visible
5. Press "Share" and verify the Share menu is visible
Other platforms
4. Verify file was downloaded to the device
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
ANDROID.mp4
Android: mWeb Chrome
ANDROID_WEB.mov
iOS: Native
IOS.mov
iOS: mWeb Safari
IOS-WEB.mov
MacOS: Chrome / Safari
Screen.Recording.2024-03-26.at.12.38.17.mov
MacOS: Desktop
Screen.Recording.2024-03-26.at.12.40.08.mov