refactor: replace stringly-typed APIs with enums across codebase - #440
Conversation
Type-safety audit: replace raw String parameters with proper enums
where the protocol defines a finite set of valid values. All changes
verified against captured WhatsApp Web JS.
HostType enum (wacore/src/iq/mediaconn.rs):
- Replace host_type: String with HostType enum (Primary | Fallback)
- Verified against WAWeb/Media/Host.js: Object.freeze({ PRIMARY, FALLBACK })
DayOfWeek + BusinessHourMode enums (wacore/src/iq/business.rs):
- Replace day_of_week: String with DayOfWeek enum (sun..sat)
- Replace mode: String with BusinessHourMode enum (open_24h | specific_hours | appointment_only)
- Verified against WAWeb/Business/ProfileTypes.js
NewsletterMessageType enum (src/features/newsletter.rs):
- Replace message_type: String with NewsletterMessageType enum
- Variants: Text, Media, Reaction, Revoke, PollCreation, PollVote, Edit + Other fallback
- Verified against WAWeb/Newsletter/MsgParser.js
DirtyType expansion + Client API (wacore/src/iq/dirty.rs, src/client.rs, src/handlers/ib.rs):
- Add SyncdAppState and NewsletterMetadata variants to DirtyType
- Change Client::clean_dirty_bits to accept DirtyBit instead of raw strings
- Handler now uses enum pattern matching instead of string comparisons
- Verified against WAWeb/Dirty/BitsConsts.js
PreKeyFetchReason enum (wacore/src/iq/prekeys.rs):
- Replace reason: Option<String> with Option<PreKeyFetchReason>
- Variants: Identity, Retry + Other fallback
- Updated all callers in src/prekeys.rs, src/client/context_impl.rs, src/client/sessions.rs
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR replaces multiple string-based parameters and fields with strongly-typed enums across the codebase (dirty bits, prekey reasons, newsletter message types, media host types, business hours, message categories, and encryption types). Call sites, parsing, IQ builders, and tests were updated to construct, compare, and serialize the new typed values. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 32s —— View job Code Review: Type-Safety Audit
Overall this is a clean, well-structured refactor. The enum definitions are correct, the Looks Good
Minor Suggestions
VerdictThis is a solid type-safety improvement. The breaking API changes are well-documented, all enums have fallback variants for forward compatibility, and the test coverage validates the changes. Approve. |
Bencher Report![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44f6ba5612
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let ts = timestamp_str.and_then(|s| s.parse::<u64>().ok()); | ||
| let bit = match ts { | ||
| Some(t) => DirtyBit::with_timestamp(dirty_type.clone(), t), | ||
| None => DirtyBit::new(dirty_type.clone()), |
There was a problem hiding this comment.
Parsing timestamp with s.parse::<u64>().ok() silently converts malformed values into None, and the code then sends clean without a timestamp. In the malformed-input path (for example, unexpected server format or overflow), this can acknowledge a dirty bit at the wrong scope and risk state divergence instead of surfacing the protocol error. Please preserve the error path (or skip cleaning that bit) rather than silently stripping the timestamp.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/ib.rs`:
- Around line 46-53: Remove the intermediate owned String allocation by parsing
the timestamp directly from the borrowed &str returned by
attrs.optional_string("timestamp"); replace the current timestamp_str mapping
and ts computation with a direct chain like
attrs.optional_string("timestamp").and_then(|s| s.parse::<u64>().ok()) to
produce ts, then continue to build DirtyBit using
DirtyType::from(dirty_type_str.as_ref()) and either
DirtyBit::with_timestamp(dirty_type.clone(), t) or
DirtyBit::new(dirty_type.clone()) as before; this eliminates the unnecessary
.to_string() allocation while keeping the same control flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c7d34aa2-10f0-4a27-ba9b-c108f8a1a052
📒 Files selected for processing (14)
src/client.rssrc/client/context_impl.rssrc/client/sessions.rssrc/features/mod.rssrc/features/newsletter.rssrc/handlers/ib.rssrc/lib.rssrc/mediaconn.rssrc/prekeys.rswacore/src/iq/business.rswacore/src/iq/dirty.rswacore/src/iq/mediaconn.rswacore/src/iq/prekeys.rswacore/src/types/user.rs
- Move dirty type checks before DirtyBit construction to avoid cloning - Default missing newsletter message type to Text instead of empty Other - Remove self-explanatory doc comments on enum definitions
Parse timestamp directly from borrowed Cow<str> instead of allocating an owned String first.
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 38s —— View job Code Review: Type-Safety Audit
Clean, well-structured refactor. The enum definitions are consistent, the Looks Good
Suggestions
VerdictSolid type-safety improvement. Breaking changes are well-documented, all enums have fallback variants, and the test suite validates the changes thoroughly. Approve ✅ |
…strings
- Use existing EncType::from_wire() in src/message.rs instead of matching
against raw "pkmsg"/"msg"/"skmsg" strings (3 sites)
- Add MessageCategory enum (Peer | Empty | Other) replacing raw "peer"
string comparisons across receipt.rs, retry.rs, stanza/receipt.rs (6 sites)
- Replace hardcoded .attr("addressing_mode", "lid") with
AddressingMode::Lid.as_str() in message.rs and privacy.rs (5 sites)
- Add TODO comments for review follow-ups: newsletter default type,
duplicate BusinessHoursConfig, from_raw() vs .ok() in IB handler
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 25s —— View job Code Review: Type-Safety Audit
Thorough review of all 22 changed files across 4 commits. This is a clean, well-executed type-safety refactor. The code is correct, consistent, and the follow-up commits properly addressed earlier review feedback. Looks Good
One Minor NoteDuplicate VerdictApprove — Solid type-safety improvement. Breaking changes are well-documented, all enums have fallback variants for forward compatibility, and test coverage validates the changes thoroughly. No issues found. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/src/iq/dirty.rs (1)
60-73: 🛠️ Refactor suggestion | 🟠 MajorPut raw parsing on
DirtyBitand give it a typed error.
Client::clean_dirty_bits()now accepts aDirtyBit, but the only strict raw parser lives onCleanDirtyBitsSpecand returnsanyhow::Error. That makes thesrc/handlers/ib.rscall site duplicate timestamp parsing, and it already diverged by silently dropping invalid timestamps. ADirtyBit::from_raw(...) -> Result<DirtyBit, DirtyBitParseError>would keep validation in one place and letCleanDirtyBitsSpec::from_raw(...)delegate to it.As per coding guidelines, "Use
thiserrorfor typed errors in Rust code" and "Useanyhowfor multi-failure functions in Rust code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/iq/dirty.rs` around lines 60 - 73, Introduce a strict raw parser on DirtyBit by adding DirtyBit::from_raw(dirty_type: &str, timestamp: Option<&str>) -> Result<DirtyBit, DirtyBitParseError> that parses the timestamp and returns a typed error implemented with thiserror (DirtyBitParseError) instead of using anyhow; move the timestamp parsing logic currently in CleanDirtyBitsSpec::from_raw into that new DirtyBit::from_raw and have CleanDirtyBitsSpec::from_raw delegate to DirtyBit::from_raw to construct its bits, then update Client::clean_dirty_bits() call sites (including src/handlers/ib.rs) to use DirtyBit::from_raw and propagate/report the typed error instead of duplicating parsing or silently dropping invalid timestamps via .ok().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/newsletter.rs`:
- Around line 595-601: Remove the TODO and add a unit test that locks the
fallback behavior: ensure when a message node is missing the "type" attribute
the parser returns NewsletterMessageType::Text. Specifically, keep the current
fallback in the parsing code that uses
msg_node.attrs.get("type")...unwrap_or(NewsletterMessageType::Text) but delete
the TODO comment, and add a test for parse_newsletter_messages_response (or a
focused test that calls the message-parsing path) that supplies a node without a
"type" attribute and asserts the resulting enum equals
NewsletterMessageType::Text to prevent regression.
In `@src/handlers/ib.rs`:
- Around line 46-48: The timestamp parsing currently swallows parse errors via
.ok(), so malformed timestamps become None and lead to creating
DirtyBit::new(dirty_type) (changing ack semantics); update the parsing of
attrs.optional_string("timestamp") to explicitly fail on parse errors instead of
using .ok()—parse the string with s.parse::<u64>() and if it Err, return or
propagate an error (reject the notification) rather than falling back to None;
apply the same change to the similar block around lines handling ts/dirty_type
(the other attrs optional_string parsing) so both places treat parse failures as
fatal rather than silent.
In `@wacore/src/iq/prekeys.rs`:
- Around line 272-274: Update the wire-format documentation to match actual
server behavior: change the comment that currently documents "<key>[3-byte BE
prekey ID]</key>" to reflect that the server sends prekey IDs as <id> nodes
(and/or accept both <id> and <key>), so it aligns with the parsing logic which
iterates all children (see mapChildren/the prekey parsing block that parses
children of <list>). Modify the comment at the wire format doc (around the
existing "<key>" note) to state that prekey IDs may appear as <id> nodes (or as
<key>, accept both) to remove the inconsistency.
In `@wacore/src/types/user.rs`:
- Around line 64-71: The BusinessHoursConfig struct in wacore/src/types/user.rs
uses String for open_time/close_time which diverges from
crate::iq::business::BusinessHoursConfig (which uses Option<String>); change the
open_time and close_time fields on the user::BusinessHoursConfig to
Option<String> to unify the shape, then update any
constructors/serialization/usage sites that construct or read
BusinessHoursConfig (search for BusinessHoursConfig, open_time, close_time) to
handle Option<String> (mapping empty-string sentinel logic to None or vice-versa
where interop is necessary) and add/adjust any unit conversions or tests to
ensure compatibility with crate::iq::business::BusinessHoursConfig.
---
Outside diff comments:
In `@wacore/src/iq/dirty.rs`:
- Around line 60-73: Introduce a strict raw parser on DirtyBit by adding
DirtyBit::from_raw(dirty_type: &str, timestamp: Option<&str>) ->
Result<DirtyBit, DirtyBitParseError> that parses the timestamp and returns a
typed error implemented with thiserror (DirtyBitParseError) instead of using
anyhow; move the timestamp parsing logic currently in
CleanDirtyBitsSpec::from_raw into that new DirtyBit::from_raw and have
CleanDirtyBitsSpec::from_raw delegate to DirtyBit::from_raw to construct its
bits, then update Client::clean_dirty_bits() call sites (including
src/handlers/ib.rs) to use DirtyBit::from_raw and propagate/report the typed
error instead of duplicating parsing or silently dropping invalid timestamps via
.ok().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 352d09ca-0c51-4b5a-94fc-bf9d12a044ff
📒 Files selected for processing (15)
src/features/newsletter.rssrc/handlers/ib.rssrc/message.rssrc/pdo.rssrc/receipt.rssrc/retry.rswacore/src/iq/business.rswacore/src/iq/dirty.rswacore/src/iq/mediaconn.rswacore/src/iq/prekeys.rswacore/src/iq/privacy.rswacore/src/messages.rswacore/src/stanza/receipt.rswacore/src/types/message.rswacore/src/types/user.rs
| // TODO: consolidate with crate::iq::business::BusinessHoursConfig which uses Option<String> | ||
| // for open_time/close_time instead of plain String. | ||
| #[derive(Debug, Clone)] | ||
| pub struct BusinessHoursConfig { | ||
| pub day_of_week: String, | ||
| pub mode: String, | ||
| pub day_of_week: crate::iq::business::DayOfWeek, | ||
| pub mode: crate::iq::business::BusinessHourMode, | ||
| pub open_time: String, | ||
| pub close_time: String, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Unify BusinessHoursConfig shape to avoid lossy conversions.
This struct still diverges from wacore/src/iq/business.rs:86-94 (String vs Option<String> for time fields), which keeps two incompatible nominal models for the same concept and can leak empty-string sentinel logic into callers.
Proposed alignment
-// TODO: consolidate with crate::iq::business::BusinessHoursConfig which uses Option<String>
-// for open_time/close_time instead of plain String.
#[derive(Debug, Clone)]
pub struct BusinessHoursConfig {
pub day_of_week: crate::iq::business::DayOfWeek,
pub mode: crate::iq::business::BusinessHourMode,
- pub open_time: String,
- pub close_time: String,
+ pub open_time: Option<String>,
+ pub close_time: Option<String>,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO: consolidate with crate::iq::business::BusinessHoursConfig which uses Option<String> | |
| // for open_time/close_time instead of plain String. | |
| #[derive(Debug, Clone)] | |
| pub struct BusinessHoursConfig { | |
| pub day_of_week: String, | |
| pub mode: String, | |
| pub day_of_week: crate::iq::business::DayOfWeek, | |
| pub mode: crate::iq::business::BusinessHourMode, | |
| pub open_time: String, | |
| pub close_time: String, | |
| #[derive(Debug, Clone)] | |
| pub struct BusinessHoursConfig { | |
| pub day_of_week: crate::iq::business::DayOfWeek, | |
| pub mode: crate::iq::business::BusinessHourMode, | |
| pub open_time: Option<String>, | |
| pub close_time: Option<String>, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/src/types/user.rs` around lines 64 - 71, The BusinessHoursConfig
struct in wacore/src/types/user.rs uses String for open_time/close_time which
diverges from crate::iq::business::BusinessHoursConfig (which uses
Option<String>); change the open_time and close_time fields on the
user::BusinessHoursConfig to Option<String> to unify the shape, then update any
constructors/serialization/usage sites that construct or read
BusinessHoursConfig (search for BusinessHoursConfig, open_time, close_time) to
handle Option<String> (mapping empty-string sentinel logic to None or vice-versa
where interop is necessary) and add/adjust any unit conversions or tests to
ensure compatibility with crate::iq::business::BusinessHoursConfig.
…m types/user.rs These types had zero references across the codebase. The canonical BusinessHoursConfig (with DayOfWeek/BusinessHourMode enums) lives in wacore/src/iq/business.rs and is the only one used.
…rekey docs DirtyBit::from_raw() with typed DirtyBitParseError (thiserror): - Moved timestamp parsing logic from CleanDirtyBitsSpec into DirtyBit::from_raw() - CleanDirtyBitsSpec::from_raw() now delegates to DirtyBit::from_raw() - IB handler now uses DirtyBit::from_raw() and rejects (warns+skips) malformed timestamps instead of silently treating them as None Newsletter: - Removed TODO comment, added unit tests locking the fallback behavior: missing type attribute defaults to NewsletterMessageType::Text Prekeys: - Fixed wire-format doc: server sends prekey IDs as <id> nodes, not <key>
Only keep PreKeyFetchReason enum. The digest <key> vs <id> fix will be done in a separate PR.
1 participant
Summary
Full type-safety audit: replace raw
Stringparameters with proper enums wherever the WhatsApp protocol defines a finite set of valid values. All changes verified against captured WhatsApp Web JS.Changes
String("primary"/"fallback")HostTypeenumWAWeb/Media/Host.jsString("sun".."sat")DayOfWeekenumWAWeb/Business/ProfileTypes.jsString("open_24h" etc.)BusinessHourModeenumWAWeb/Business/ProfileTypes.jsString("text"/"media" etc.)NewsletterMessageTypeenumWAWeb/Newsletter/MsgParser.jsDirtyType(missing variants)SyncdAppState,NewsletterMetadataWAWeb/Dirty/BitsConsts.js(&str, Option<&str>)DirtyBit(typed struct)Option<String>Option<PreKeyFetchReason>enumWAWeb/Wam/EnumPrekeysFetchContext.jsBreaking changes
Client::clean_dirty_bits()now acceptsDirtyBitinstead of(&str, Option<&str>)MediaConnHost.host_typeandMediaConnHostExtended.host_typechanged fromStringtoHostTypeBusinessHoursConfig.day_of_week/.modechanged fromStringto enumsNewsletterMessage.message_typechanged fromStringtoNewsletterMessageTypePreKeyFetchSpec.reasonchanged fromOption<String>toOption<PreKeyFetchReason>14 files changed, 216 additions, 67 deletions.
Test plan
cargo test -p wacore --lib -- iq::mediaconn iq::dirty iq::prekeys iq::business)cargo clippy --all --testsclean (0 warnings)Summary by CodeRabbit
New Features
Improvements
Bug Fixes