debug(harmony): log constants returned by getConstants on native side#595
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesgetConstants result logging
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
harmony/pushy/src/main/ets/PushyTurboModule.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: .eslintrc.js » Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
harmony/pushy/src/main/ets/PushyTurboModule.ts (1)
109-110: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider preserving
undefinedvalues in the debug log for accurate native-to-JS contract verification.
JSON.stringifydrops keys withundefinedvalues, so ifrolledBackVersion(or any future field) isundefined, the log output won't match the actual object shape returned to JS. Since this log's purpose is to verify the exact constants contract, consider normalizingundefinedtonullin the logged object so omitted keys don't mislead investigation:- logger.debug(TAG, `,getConstants result: ${JSON.stringify(result)}`); + logger.debug(TAG, `,getConstants result: ${JSON.stringify(result, (_k, v) => v === undefined ? null : v)}`);Or explicitly normalize
rolledBackVersiontonullwhen constructingresultif that aligns with the Android contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@harmony/pushy/src/main/ets/PushyTurboModule.ts` around lines 109 - 110, The debug log in PushyTurboModule.getConstants is misleading because JSON.stringify drops undefined fields, so the logged shape may not match the actual JS contract. Update the getConstants result logging to preserve undefined-vs-null semantics by normalizing undefined fields (such as rolledBackVersion) to null in the logged object, or by aligning the returned result shape in getConstants so the debug output accurately reflects what the native module exposes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@harmony/pushy/src/main/ets/PushyTurboModule.ts`:
- Around line 109-110: The debug log in PushyTurboModule.getConstants is
misleading because JSON.stringify drops undefined fields, so the logged shape
may not match the actual JS contract. Update the getConstants result logging to
preserve undefined-vs-null semantics by normalizing undefined fields (such as
rolledBackVersion) to null in the logged object, or by aligning the returned
result shape in getConstants so the debug output accurately reflects what the
native module exposes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed250bcb-8578-4112-9d5f-18aee4dfee18
📒 Files selected for processing (1)
harmony/pushy/src/main/ets/PushyTurboModule.ts
763df2a to
dc06843
Compare
This PR adds a native-side debug log statement to print the exact constants returned by getConstants() inside
PushyTurboModule.ts:This will help verify what constants are returned by the native side to the JS engine (and check for any synchronization or timing issues in Bridgeless/React Native 0.77 environments).
Summary by CodeRabbit