Skip to content

Conversation

@paulr34
Copy link
Collaborator

@paulr34 paulr34 commented Aug 5, 2025

  • Added emoji-util.js utility for conditional emoji formatting
  • Updated args.js to include --no-emoji command line option
  • Modified script-util.js to use formatMessage() for all emoji output
  • Updated startup.js to use emoji utility
  • Added documentation and demo scripts
  • Resolves issues with stdout when piping to automation tools
  • Python no longer chokes on emoji characters in output
  • Maintains backward compatibility (emojis enabled by default)

- Added emoji-util.js utility for conditional emoji formatting
- Updated args.js to include --no-emoji command line option
- Modified script-util.js to use formatMessage() for all emoji output
- Updated startup.js to use emoji utility
- Added documentation and demo scripts
- Resolves issues with stdout when piping to automation tools
- Python no longer chokes on emoji characters in output
- Maintains backward compatibility (emojis enabled by default)
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

Hello @paulr34, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

I've implemented a new --no-emoji command-line option to disable emoji characters in console output. This change addresses issues where emojis could cause problems when piping output to automation tools or systems with limited character set support, such as Python scripts. By default, emojis remain enabled, ensuring backward compatibility while providing a clean text-only output option.

Highlights

  • New Command-Line Option: Added --no-emoji to allow users to disable emoji characters in console output.
  • Centralized Emoji Formatting: Introduced a new utility file, emoji-util.js, which provides a formatMessage function to conditionally include emojis based on the --no-emoji flag or NO_EMOJI environment variable.
  • Argument Parsing Integration: Updated args.js to recognize and process the --no-emoji command-line option, setting an internal environment variable.
  • Console Output Adaptation: Modified startup.js and script-util.js to utilize the new formatMessage utility for all console output that previously contained emojis, ensuring consistent behavior.
  • Documentation and Demos: Included comprehensive documentation in docs/no-emoji-implementation.md and demonstration scripts (demo-no-emoji.js, test-emoji.js) to illustrate the feature's usage and functionality.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a valuable --no-emoji option to improve compatibility with automation tools. The core idea is well-implemented in emoji-util.js. However, there are several critical issues that need to be addressed. The changes in startup.js will cause a runtime error due to calling a non-existent function. The script-util.js file appears to be corrupted, likely from a merge conflict. Additionally, the new test file is broken and relies on functions that haven't been implemented, and the documentation is out of sync with the code. These issues must be fixed before this PR can be merged.

paulr34 and others added 12 commits August 5, 2025 16:00
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
- Fixed corrupted license header in script-util.js
- Standardized to use emojiUtil.formatMessage() in script files
- Keep env.formatMessage() wrapper for startup.js compatibility
- Both approaches work correctly for their respective module systems
- Fix broken emoji syntax in startup.js and gsdk-public-regen.js
- Add emoji support to gsdk-public-regen.js and zap-package-metadata.js
- Create fix-emojis.js utility script for systematic emoji conversion
- All emojis now respect --no-emoji flag and NO_EMOJI environment variable
- Add emoji utility to zap-start.js and zap-uitest.js
- Fix remaining hardcoded emojis in zap-start.js, zap-uitest.js, and gsdk-public-regen.js
- All console output now respects --no-emoji flag and NO_EMOJI environment variable
- Comprehensive solution ensures automation-friendly output for CI/CD pipelines
- Replace static constant with dynamic isEmojiDisabled() function
- Add setEmojiDisabled() and resetEmojiState() for testing
- Export testing functions through env.js wrapper
- Fix test to use CommonJS emoji-util directly
- Maintains environment variable and command line flag detection
- Enables proper unit testing of emoji functionality
- Restore docs/api.md from commit 48de006 (before emoji changes)
- File was accidentally truncated in commit 5f8ef3e due to JSDoc generation failure
- JSDoc fails with Node.js v24 due to util.isRegExp compatibility issue
- Restores all 28,406 lines of API documentation
- Remove redundant summary file
- Documentation is already covered in docs/no-emoji-implementation.md
- Remove demo-no-emoji.js (was just a demonstration script)
- Remove fix-emojis.js (was a utility script for development)
- Keep only the production implementation files
- Remove duplicate formatMessage function from emoji-util.js
- Keep single formatMessage implementation in env.js for general message formatting
- Update all imports and function calls across codebase from emojiUtil.formatMessage to env.formatMessage
- Maintain emoji-util.js focus on emoji state management only
- Improve code organization by placing message formatting in general utilities module

Fixes code duplication and follows best practices for single responsibility principle.
- Convert 42 'export function' declarations to regular functions
- Convert 'export const environmentVariable' to regular const
- Add all exports to module.exports object for CommonJS compatibility
- Resolves CI failure caused by mixed ES/CommonJS module syntax
- Maintains all existing functionality while fixing Node.js module loading
- Implement formatMessage logic directly instead of calling non-existent emojiUtil.formatMessage
- Use emojiUtil.isEmojiDisabled() to check state and format accordingly
- Resolves TypeError: emojiUtil.formatMessage is not a function
Copy link
Collaborator

@ethanzhouyc ethanzhouyc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with comments

paulr34 added 2 commits August 6, 2025 16:12
…ormatEmojiMessage

- Fix corrupted emojis (� symbols) that appeared due to encoding issues during sed operations
- Rename formatMessage() to formatEmojiMessage() for better semantic clarity
- Restore proper Unicode emojis: 👈 for read operations, 👉 for write operations, 🔧 for processing
- Update all 94+ function call sites across startup.js, script utilities, and tests
- Verify emoji functionality works correctly with comprehensive test coverage
- Remove obsolete test files and documentation per cleanup requirements

Addresses feedback from PR review regarding function naming and emoji display issues.
Remove empty files that were created during development:
- EMOJI_IMPLEMENTATION_SUMMARY.md
- demo-no-emoji.js
- fix-emojis.js

These were temporary files that never got populated with content.
Copy link
Collaborator

@brdandu brdandu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving this but please clean up the PR before merging. Not sure if this is working with Studio. Leaving the testing part with you.

}
options.logger(` 👉 write out: ${upgrade_results}`)
options.logger(
env.formatEmojiMessage('🔧', `👉 write out: ${upgrade_results}`)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'🔧' ?

type: 'boolean',
deafult: false
})
.option('noEmoji', {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we have 2 different formats?
noEmoji and no-emoji. very confused.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not seeing no-emoji here?


// Set emoji preference via environment variable
if (ret.noEmoji) {
process.env.NO_EMOJI = '1'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best to use the noEmoji option coming from command args i.e. the uc_generate target from pack.json instead of doing a 2 step process of setting this as a environment.

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 82.66667% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.90%. Comparing base (dc0747a) to head (43e6b24).
⚠️ Report is 48 commits behind head on master.

Files with missing lines Patch % Lines
src-electron/main-process/startup.js 60.78% 20 Missing ⚠️
src-electron/util/env.js 94.04% 5 Missing ⚠️
src-electron/util/args.js 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1635      +/-   ##
==========================================
- Coverage   66.93%   66.90%   -0.03%     
==========================================
  Files         197      201       +4     
  Lines       21827    22794     +967     
  Branches     4817     5073     +256     
==========================================
+ Hits        14609    15250     +641     
- Misses       7218     7544     +326     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

4 participants