Enabled test cases, switched flag bits to bigint for precision, and added better permission handling#23
Merged
blstmo merged 5 commits intofluxerjs:mainfrom Feb 24, 2026
Merged
Conversation
… keys, not serialized bigints (ie. "123")
…ermissions` properties
|
@MusicMakerOwO is attempting to deploy a commit to the blstmo's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughA comprehensive refactor migrating bitfield implementations from numeric types to BigInt, restructuring permission flag representations from enums to const objects, and updating permission-related APIs across Role, GuildMember, and utilities to use BitField-based permission access patterns instead of method-based checks. Test imports standardized to use barrel exports, and message utilities converted to async operation patterns. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/util/src/MessageFlagsBitField.ts (1)
3-6:⚠️ Potential issue | 🟡 MinorStale JSDoc — "Format: int32" no longer accurate.
The comment describes the API schema type but now clashes with the bigint representation; readers may be confused about whether the JS values are
numberorbigint.📝 Suggested fix
/** * Message flag bit values matching the API MessageFlags schema (openapi.json). - * Format: int32. + * Values are bigint; bit positions match the API's int32 MessageFlags field. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/util/src/MessageFlagsBitField.ts` around lines 3 - 6, Update the stale JSDoc in MessageFlagsBitField.ts to remove "Format: int32" and clearly state that flag values are represented as bigint in this implementation; locate the top-of-file comment above the MessageFlagsBitField (or the exported bitfield constants) and replace the inaccurate schema type note with a short note like "Values represented as bigint" or "Uses bigint flag values" so readers know these are bigint JS values rather than 32-bit numbers.packages/fluxer-core/src/structures/GuildMember.ts (1)
154-183:⚠️ Potential issue | 🟠 Major
new BitField<…>(perms)should benew PermissionsBitField(perms)— string-based flag checks will throw.
BitField.Flagsdefaults to{}. Returning a plainBitFieldmeansmember.permissions.has('SendMessages')will fail at runtime (string not found inFlags, numeric parse also fails →RangeError). UsingPermissionsBitFieldwires upPermissionFlagsviastatic override Flags, enabling both bigint and string-key checks.🔧 Proposed fix
In the import block:
import { BitField, - PermissionFlags + PermissionsBitField, + PermissionFlags, } from '@fluxerjs/util';In
get permissions()andpermissionsIn():- return new BitField<keyof typeof PermissionFlags>(perms); + return new PermissionsBitField(perms);(apply the same change on both line 159 and line 182)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fluxer-core/src/structures/GuildMember.ts` around lines 154 - 183, Replace the generic BitField construction with the permissions-aware PermissionsBitField and import it: in the file's imports add/replace to bring in PermissionsBitField, then in the getter permissions() (method name: permissions) and the method permissionsIn(channel: GuildChannel) replace new BitField<keyof typeof PermissionFlags>(perms) with new PermissionsBitField(perms) so string-based flag checks (e.g. member.permissions.has('SendMessages')) use the correct static Flags mapping.packages/util/src/UserFlagsBitField.ts (1)
4-7:⚠️ Potential issue | 🟡 MinorStale JSDoc — the 32-bit caveat no longer applies.
The comment warns that
BitFielduses 32-bit JS bitwise operations, which was the bug this PR fixes. The note should be removed now that the whole class operates on bigint.📝 Suggested fix
/** * User flag bit values matching the API UserFlags schema (openapi.json). - * Values are int64; bit shifts used where within 32-bit range, literals otherwise. - * Note: BitField's has/add/remove use JS bitwise ops (32-bit); full values are for API serialization. + * Values are bigint to handle flags that exceed the 32-bit range (e.g. bits 33–57). */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/util/src/UserFlagsBitField.ts` around lines 4 - 7, The JSDoc at the top of UserFlagsBitField is stale: remove the sentence that warns about BitField using 32-bit JS bitwise ops and update the wording to reflect that the class now operates on bigint; specifically, in the header comment for UserFlagsBitField (which references BitField), delete the line "BitField's has/add/remove use JS bitwise ops (32-bit); full values are for API serialization." and replace or adjust the preceding sentence so it clearly states values are int64 represented as bigint (use bigint literals/shifts) and that the class uses bigint operations for correctness.packages/fluxer-core/src/util/permissions.ts (1)
22-33:⚠️ Potential issue | 🟡 MinorOverwrite processing order matters — role and member overwrites are not separated.
The single-pass loop applies each overwrite sequentially, so the final result depends on the array order. If a role-allow appears after a member-deny for the same bit, the role-allow will re-add the permission — contrary to the typical convention (member overwrites should always take precedence over role overwrites regardless of array order).
Discord's algorithm processes role overwrites and member overwrites in separate passes to ensure member overwrites always win. Consider splitting into two passes:
♻️ Suggested two-pass approach
if (isOwner) return ALL_PERMISSIONS_BIGINT; let perms = basePermissions; - for (const overwrite of overwrites ?? []) { - const applies = - (overwrite.type === OverwriteType.Role && memberRoles.includes(overwrite.id)) || - (overwrite.type === OverwriteType.Member && overwrite.id === memberId); - if (!applies) continue; + // Pass 1: apply role overwrites + for (const overwrite of overwrites ?? []) { + if (overwrite.type !== OverwriteType.Role || !memberRoles.includes(overwrite.id)) continue; + const allow = BigInt(overwrite.allow || '0'); + const deny = BigInt(overwrite.deny || '0'); + perms = (perms & ~deny) | allow; + } + // Pass 2: apply member overwrites (take precedence) + for (const overwrite of overwrites ?? []) { + if (overwrite.type !== OverwriteType.Member || overwrite.id !== memberId) continue; const allow = BigInt(overwrite.allow || '0'); const deny = BigInt(overwrite.deny || '0'); perms = (perms & ~deny) | allow; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fluxer-core/src/util/permissions.ts` around lines 22 - 33, The current single-pass loop over overwrites lets array order determine final permissions, violating Discord semantics where member overwrites must override role overwrites; change the logic in the permission calculation (the block using isOwner, basePermissions, overwrites, OverwriteType.Role, OverwriteType.Member, memberRoles, memberId, and PermissionFlags.Administrator) to perform two passes: first iterate only OverwriteType.Role entries that apply to the member and apply their deny/allow to perms, then iterate only OverwriteType.Member entries that apply and apply their deny/allow so member-level denies/allows always win; keep the same bit operations ((perms & ~deny) | allow) and final Administrator check returning ALL_PERMISSIONS_BIGINT.
🧹 Nitpick comments (10)
packages/fluxer-core/src/structures/Invite.test.ts (1)
4-6: Consider a sturdier Client mock to avoid future brittleness.
Casting{}toClientmay break ifInvitestarts accessing client fields during construction. A minimal mock with the required properties keeps tests resilient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fluxer-core/src/structures/Invite.test.ts` around lines 4 - 6, The current createMockClient() returns an empty object cast to Client which is brittle; update createMockClient to return a minimal mock object (e.g., use Partial<Client> and fill in the specific properties and methods that Invite's constructor or methods access) instead of {} as Client so tests don't break if Invite reads client fields during construction; locate createMockClient and ensure it provides the concrete properties/methods (names used by Invite) with simple stubs before casting to Client for the test.packages/fluxer-core/src/util/messageUtils.test.ts (2)
11-14: Verbatim duplicate ofMessage.test.tslines 48–51.Since both files now test
Message._createMessageBody, consider partitioning responsibility: keep reply/reference/validation cases inMessage.test.tsand embed/attachment/edge-case handling here, removing duplicated scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fluxer-core/src/util/messageUtils.test.ts` around lines 11 - 14, The test in messageUtils.test.ts duplicates the Message._createMessageBody coverage already present in Message.test.ts; remove the duplicate case that checks passthrough content and instead focus messageUtils.test.ts on complementary scenarios (embed handling, attachments, and edge-case inputs like empty content or invalid options) while retaining reply/reference/validation tests in Message.test.ts; locate the duplicated test by the Message._createMessageBody call and modify messageUtils.test.ts to assert embed/attachment/edge-case behaviors instead.
6-8: StricttoEqualon the full body shape is brittle.If the implementation ever adds a default field (e.g.,
tts: false), this assertion fails without a real regression. Prefer checking only the fields under test:♻️ Suggested narrowing
- expect(result.body).toEqual({ content: 'Hello world' }); + expect(result.body.content).toBe('Hello world');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fluxer-core/src/util/messageUtils.test.ts` around lines 6 - 8, The test for Message._createMessageBody is too strict by asserting the entire body shape; update the assertion to only check the fields under test (e.g., assert result.body.content === 'Hello world' or use expect(result.body).toEqual(expect.objectContaining({ content: 'Hello world' })) ) so adding default fields (like tts) won't break the test while still validating the content behavior of Message._createMessageBody.packages/fluxer-core/src/structures/Message.test.ts (1)
48-51: Duplicate test case — also present verbatim inmessageUtils.test.ts.The "passes through content from options" test (and parts of the
filesdescribe block) is replicated across both test files. Since both files now coverMessage._createMessageBody, consider consolidating overlapping cases into one file and keeping the other focused on its unique scenarios (e.g., reply/reference semantics here, embed serialization inmessageUtils.test.ts).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fluxer-core/src/structures/Message.test.ts` around lines 48 - 51, The test "passes through content from options" duplicates coverage in messageUtils.test.ts; remove the redundant case from packages/fluxer-core/src/structures/Message.test.ts (the it block referencing Message._createMessageBody with content 'Test') and consolidate shared _createMessageBody tests into one canonical file (keep embed serialization tests in messageUtils.test.ts), while preserving Message.test.ts for its unique reply/reference and files describe block scenarios (adjust or trim the 'files' describe block here to only include cases not already covered in messageUtils.test.ts).packages/util/src/BitField.ts (2)
17-17:defaultBit = 0is a stalenumberfield — unused in the bigint implementation.It was part of the pre-bigint design. Now that everything is
bigint, this field is dead weight and itsnumbertype is misleading.♻️ Suggested removal
export class BitField<S extends string> { static Flags: Record<string, bigint> = {}; - defaultBit = 0; bitfield: bigint;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/util/src/BitField.ts` at line 17, Remove the stale numeric field defaultBit from the BitField class (it’s a leftover from the pre-bigint implementation); locate the declaration "defaultBit = 0" in BitField and delete it and any references/usages to defaultBit (ensure constructors and methods like the BitField constructor, set/has/toggle helpers, or static helpers rely only on bigint fields such as the internal bit value and bigint masks), and run tests/type-check to confirm no remaining references to defaultBit remain.
32-32: RedundantBigInt()wrapping in the array reduce —mapalready returnsbigint[].
this.resolve(b)returnsbigint, so the accumulator and each element in the reduce are alreadybigint. Wrapping withBigInt()again is a no-op but adds noise.♻️ Suggested simplification
- if (Array.isArray(bits)) return bits.map((b) => this.resolve(b)).reduce((a, b) => BigInt(a) | BigInt(b), 0n); + if (Array.isArray(bits)) return bits.map((b) => this.resolve(b)).reduce((a, b) => a | b, 0n);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/util/src/BitField.ts` at line 32, The reduce is redundantly wrapping values with BigInt() even though this.resolve returns bigint; update the Array.isArray branch in BitField.ts to avoid the extra conversions—e.g. keep bits.map(b => this.resolve(b)) but change .reduce((a, b) => BigInt(a) | BigInt(b), 0n) to .reduce((a, b) => a | b, 0n) (or collapse to a single reduce using this.resolve inside) so the accumulator and elements remain bigint without needless BigInt() calls.packages/fluxer-core/src/structures/Role.ts (1)
46-49: Getter allocates a newPermissionsBitFieldon every access.Each call to
role.permissionsconstructs a newPermissionsBitField. This is fine for occasional access, but could become a concern if called in tight loops (e.g., iterating all roles and checking permissions repeatedly). Consider caching and invalidating in_patchif this becomes a hot path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fluxer-core/src/structures/Role.ts` around lines 46 - 49, The permissions getter currently allocates a new PermissionsBitField on every access (in get permissions), which can be costly in hot paths; change it to cache the constructed PermissionsBitField instance (e.g., store it on the Role instance like this._cachedPermissions) and return the cached instance when _permissions hasn't changed, and invalidate or rebuild that cache inside the _patch method whenever this._permissions is updated; ensure you still apply the Administrator mask logic (PermissionFlags.Administrator -> ALL_PERMISSIONS_BIGINT) when populating the cached PermissionsBitField.packages/util/src/PermissionsBitField.ts (1)
73-98:resolvePermissionsToBitfieldsilently resolves unknown string elements to0n.On Line 91, if
pis a string that's not inPermissionFlagsand not a valid number,Number(p)returnsNaN, andNaN || 0evaluates to0, soBigInt(0)silently contributes nothing to the accumulated bitfield. This masks typos or invalid permission names inside arrays.Consider throwing a
RangeErrorfor invalid array elements, consistent with the top-level string handling on Line 79.♻️ Proposed fix
else if (typeof p === 'string') { const mapped = PermissionFlags[p]; - v = mapped !== undefined ? BigInt(mapped) : BigInt(Number(p) || 0); + if (mapped !== undefined) { + v = BigInt(mapped); + } else { + const num = Number(p); + if (Number.isNaN(num) || p.trim() === '') throw new RangeError(`Invalid permission string in array: ${p}`); + v = BigInt(num); + } } else v = BigInt((p as PermissionsBitField).bitfield);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/util/src/PermissionsBitField.ts` around lines 73 - 98, The resolvePermissionsToBitfield function currently treats unknown string elements in arrays as 0n (silently ignoring typos); update the array branch in resolvePermissionsToBitfield so that when an element p is a string and PermissionFlags[p] is undefined you explicitly check if Number(p) is a valid finite number (not NaN) before converting to BigInt, and if it is not a valid number throw a RangeError (e.g. `Invalid permission string: ${p}`) instead of falling back to 0; ensure the error behavior matches the top-level string case and keep handling for number, bigint and PermissionsBitField elements unchanged.packages/fluxer-core/src/util/permissions.test.ts (2)
79-88: Test relies on array ordering for "member overwrite wins" assertion.This test passes because the member-deny overwrite appears after the role-allow in the array. If the order were swapped, the assertion would fail. This is fragile — consider either documenting the ordering requirement clearly or adding a test with reversed order to catch regressions if
computePermissionsis later fixed to use a two-pass algorithm.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fluxer-core/src/util/permissions.test.ts` around lines 79 - 88, The test for "member overwrite wins" is fragile because it depends on overwrites array order; add a second test case that swaps the two overwrite entries (role then member vs member then role) to ensure computePermissions produces the same result regardless of ordering, referencing computePermissions, hasPermission, OverwriteType, and PermissionFlags; alternatively, if ordering is expected, add a clarifying comment in the existing test naming that computePermissions requires member entries to come after role entries, but preferred fix is to add the reversed-order test asserting hasPermission(perms, PermissionFlags.SendMessages) is false for both orders.
39-41: RedundantBigInt()wrapping —PermissionFlags.*values are already bigints.
PermissionFlags.ManageRolesis1n << 28n(abigint). Wrapping it inBigInt()is a no-op. Same forPinMessagesandBypassSlowmodeon Lines 40–41.♻️ Suggested cleanup
- expect(hasPermission(perms, BigInt(PermissionFlags.ManageRoles))).toBe(true); - expect(hasPermission(perms, BigInt(PermissionFlags.PinMessages))).toBe(true); - expect(hasPermission(perms, BigInt(PermissionFlags.BypassSlowmode))).toBe(true); + expect(hasPermission(perms, PermissionFlags.ManageRoles)).toBe(true); + expect(hasPermission(perms, PermissionFlags.PinMessages)).toBe(true); + expect(hasPermission(perms, PermissionFlags.BypassSlowmode)).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fluxer-core/src/util/permissions.test.ts` around lines 39 - 41, The assertions wrap PermissionFlags values in BigInt() unnecessarily; update the three expectations to pass the bigint constants directly (remove BigInt(...) around PermissionFlags.ManageRoles, PermissionFlags.PinMessages, and PermissionFlags.BypassSlowmode) so hasPermission(perms, PermissionFlags.ManageRoles), hasPermission(perms, PermissionFlags.PinMessages), and hasPermission(perms, PermissionFlags.BypassSlowmode) use the existing bigint constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/fluxer-core/src/structures/GuildMember.ts`:
- Around line 185-195: The permission constants in PermissionsBitField.ts use
incorrect bases like `2n << N` shifting bits one position; update the flagged
constants (`UseExternalStickers`, `ModerateMembers`, `CreateExpressions`,
`PinMessages`, `BypassSlowmode`, `UpdateRtcRegion`) to use `1n << N` with the
correct N values (e.g., `1n << 37n`, `1n << 40n`, etc.), and review/fix the
duplicate alias where both `ManageEmojisAndStickers` and `ManageExpressions` are
defined as `1n << 30n` (ensure each permission has the correct unique bit). This
will correct bit positions used by `_computeBasePermissions()` and
`permissionsIn()`.
In `@packages/util/src/BitField.ts`:
- Line 38: Remove the debug console.log statements that print internal state:
delete the console.log(this.Flags) call inside the resolve() method and the
console.log used inside the has() method (which logs each bit check); these
debug prints should be removed from the BitField class (referencing resolve(),
has(), and the Flags property) so production output is not polluted—if you need
retained diagnostics, replace with a controlled logger at a debug level instead
of console.log.
- Around line 51-55: The has method currently logs the input and uses any-bit
semantics ((this.bitfield & bit) !== 0n); remove the debug console.log and
change the check to require all bits are present by comparing equality to the
resolved bit mask ((this.bitfield & bit) === bit). Update the implementation in
the BitField.has method and ensure you use BitField.resolve (this.constructor as
typeof BitField).resolve to obtain the mask so behavior matches
serialize/toArray.
- Around line 40-46: The code in BitField.ts accepts numeric string literals via
BigInt(bits) (see the BigInt(bits) fallback that returns a non-negative bigint),
but tests in BitField.test.ts don't cover this path; either add a unit test
asserting that passing a numeric string like "274877906944" yields the expected
bigint/bitfield behavior, or remove the numeric-string fallback if it's
unintended. Locate the BigInt(bits) branch in BitField.ts (the try/catch around
BigInt) and either (A) add a test in BitField.test.ts that calls the same public
API (constructor/parse method that accepts `bits`) with a numeric string and
asserts the resulting value/mask, or (B) remove the try/catch BigInt fallback so
non-property strings throw as before—update tests accordingly to reflect the
chosen behavior.
In `@packages/util/src/PermissionsBitField.ts`:
- Around line 44-49: The bit flags UseExternalStickers, ModerateMembers,
CreateExpressions, PinMessages, BypassSlowmode and UpdateRtcRegion are using `2n
<< Xn` which sets bit X+1; change each to `1n << Xn` (e.g. replace `2n << 37n`
with `1n << 37n`) so the flags represent the intended bits, and update/confirm
the doc comment about "bits 31+ use 2**n" remains consistent; verify the
corrected values against the Fluxer API constants (e.g. Channel.ts) to ensure
they match the canonical bit positions.
---
Outside diff comments:
In `@packages/fluxer-core/src/structures/GuildMember.ts`:
- Around line 154-183: Replace the generic BitField construction with the
permissions-aware PermissionsBitField and import it: in the file's imports
add/replace to bring in PermissionsBitField, then in the getter permissions()
(method name: permissions) and the method permissionsIn(channel: GuildChannel)
replace new BitField<keyof typeof PermissionFlags>(perms) with new
PermissionsBitField(perms) so string-based flag checks (e.g.
member.permissions.has('SendMessages')) use the correct static Flags mapping.
In `@packages/fluxer-core/src/util/permissions.ts`:
- Around line 22-33: The current single-pass loop over overwrites lets array
order determine final permissions, violating Discord semantics where member
overwrites must override role overwrites; change the logic in the permission
calculation (the block using isOwner, basePermissions, overwrites,
OverwriteType.Role, OverwriteType.Member, memberRoles, memberId, and
PermissionFlags.Administrator) to perform two passes: first iterate only
OverwriteType.Role entries that apply to the member and apply their deny/allow
to perms, then iterate only OverwriteType.Member entries that apply and apply
their deny/allow so member-level denies/allows always win; keep the same bit
operations ((perms & ~deny) | allow) and final Administrator check returning
ALL_PERMISSIONS_BIGINT.
In `@packages/util/src/MessageFlagsBitField.ts`:
- Around line 3-6: Update the stale JSDoc in MessageFlagsBitField.ts to remove
"Format: int32" and clearly state that flag values are represented as bigint in
this implementation; locate the top-of-file comment above the
MessageFlagsBitField (or the exported bitfield constants) and replace the
inaccurate schema type note with a short note like "Values represented as
bigint" or "Uses bigint flag values" so readers know these are bigint JS values
rather than 32-bit numbers.
In `@packages/util/src/UserFlagsBitField.ts`:
- Around line 4-7: The JSDoc at the top of UserFlagsBitField is stale: remove
the sentence that warns about BitField using 32-bit JS bitwise ops and update
the wording to reflect that the class now operates on bigint; specifically, in
the header comment for UserFlagsBitField (which references BitField), delete the
line "BitField's has/add/remove use JS bitwise ops (32-bit); full values are for
API serialization." and replace or adjust the preceding sentence so it clearly
states values are int64 represented as bigint (use bigint literals/shifts) and
that the class uses bigint operations for correctness.
---
Nitpick comments:
In `@packages/fluxer-core/src/structures/Invite.test.ts`:
- Around line 4-6: The current createMockClient() returns an empty object cast
to Client which is brittle; update createMockClient to return a minimal mock
object (e.g., use Partial<Client> and fill in the specific properties and
methods that Invite's constructor or methods access) instead of {} as Client so
tests don't break if Invite reads client fields during construction; locate
createMockClient and ensure it provides the concrete properties/methods (names
used by Invite) with simple stubs before casting to Client for the test.
In `@packages/fluxer-core/src/structures/Message.test.ts`:
- Around line 48-51: The test "passes through content from options" duplicates
coverage in messageUtils.test.ts; remove the redundant case from
packages/fluxer-core/src/structures/Message.test.ts (the it block referencing
Message._createMessageBody with content 'Test') and consolidate shared
_createMessageBody tests into one canonical file (keep embed serialization tests
in messageUtils.test.ts), while preserving Message.test.ts for its unique
reply/reference and files describe block scenarios (adjust or trim the 'files'
describe block here to only include cases not already covered in
messageUtils.test.ts).
In `@packages/fluxer-core/src/structures/Role.ts`:
- Around line 46-49: The permissions getter currently allocates a new
PermissionsBitField on every access (in get permissions), which can be costly in
hot paths; change it to cache the constructed PermissionsBitField instance
(e.g., store it on the Role instance like this._cachedPermissions) and return
the cached instance when _permissions hasn't changed, and invalidate or rebuild
that cache inside the _patch method whenever this._permissions is updated;
ensure you still apply the Administrator mask logic
(PermissionFlags.Administrator -> ALL_PERMISSIONS_BIGINT) when populating the
cached PermissionsBitField.
In `@packages/fluxer-core/src/util/messageUtils.test.ts`:
- Around line 11-14: The test in messageUtils.test.ts duplicates the
Message._createMessageBody coverage already present in Message.test.ts; remove
the duplicate case that checks passthrough content and instead focus
messageUtils.test.ts on complementary scenarios (embed handling, attachments,
and edge-case inputs like empty content or invalid options) while retaining
reply/reference/validation tests in Message.test.ts; locate the duplicated test
by the Message._createMessageBody call and modify messageUtils.test.ts to assert
embed/attachment/edge-case behaviors instead.
- Around line 6-8: The test for Message._createMessageBody is too strict by
asserting the entire body shape; update the assertion to only check the fields
under test (e.g., assert result.body.content === 'Hello world' or use
expect(result.body).toEqual(expect.objectContaining({ content: 'Hello world' }))
) so adding default fields (like tts) won't break the test while still
validating the content behavior of Message._createMessageBody.
In `@packages/fluxer-core/src/util/permissions.test.ts`:
- Around line 79-88: The test for "member overwrite wins" is fragile because it
depends on overwrites array order; add a second test case that swaps the two
overwrite entries (role then member vs member then role) to ensure
computePermissions produces the same result regardless of ordering, referencing
computePermissions, hasPermission, OverwriteType, and PermissionFlags;
alternatively, if ordering is expected, add a clarifying comment in the existing
test naming that computePermissions requires member entries to come after role
entries, but preferred fix is to add the reversed-order test asserting
hasPermission(perms, PermissionFlags.SendMessages) is false for both orders.
- Around line 39-41: The assertions wrap PermissionFlags values in BigInt()
unnecessarily; update the three expectations to pass the bigint constants
directly (remove BigInt(...) around PermissionFlags.ManageRoles,
PermissionFlags.PinMessages, and PermissionFlags.BypassSlowmode) so
hasPermission(perms, PermissionFlags.ManageRoles), hasPermission(perms,
PermissionFlags.PinMessages), and hasPermission(perms,
PermissionFlags.BypassSlowmode) use the existing bigint constants.
In `@packages/util/src/BitField.ts`:
- Line 17: Remove the stale numeric field defaultBit from the BitField class
(it’s a leftover from the pre-bigint implementation); locate the declaration
"defaultBit = 0" in BitField and delete it and any references/usages to
defaultBit (ensure constructors and methods like the BitField constructor,
set/has/toggle helpers, or static helpers rely only on bigint fields such as the
internal bit value and bigint masks), and run tests/type-check to confirm no
remaining references to defaultBit remain.
- Line 32: The reduce is redundantly wrapping values with BigInt() even though
this.resolve returns bigint; update the Array.isArray branch in BitField.ts to
avoid the extra conversions—e.g. keep bits.map(b => this.resolve(b)) but change
.reduce((a, b) => BigInt(a) | BigInt(b), 0n) to .reduce((a, b) => a | b, 0n) (or
collapse to a single reduce using this.resolve inside) so the accumulator and
elements remain bigint without needless BigInt() calls.
In `@packages/util/src/PermissionsBitField.ts`:
- Around line 73-98: The resolvePermissionsToBitfield function currently treats
unknown string elements in arrays as 0n (silently ignoring typos); update the
array branch in resolvePermissionsToBitfield so that when an element p is a
string and PermissionFlags[p] is undefined you explicitly check if Number(p) is
a valid finite number (not NaN) before converting to BigInt, and if it is not a
valid number throw a RangeError (e.g. `Invalid permission string: ${p}`) instead
of falling back to 0; ensure the error behavior matches the top-level string
case and keep handling for number, bigint and PermissionsBitField elements
unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
packages/fluxer-core/package.jsonpackages/fluxer-core/src/structures/Guild.test.tspackages/fluxer-core/src/structures/GuildMember.test.tspackages/fluxer-core/src/structures/GuildMember.tspackages/fluxer-core/src/structures/Invite.test.tspackages/fluxer-core/src/structures/Message.test.tspackages/fluxer-core/src/structures/Role.test.tspackages/fluxer-core/src/structures/Role.tspackages/fluxer-core/src/structures/User.test.tspackages/fluxer-core/src/util/messageUtils.test.tspackages/fluxer-core/src/util/permissions.test.tspackages/fluxer-core/src/util/permissions.tspackages/util/src/BitField.test.tspackages/util/src/BitField.tspackages/util/src/MessageFlagsBitField.test.tspackages/util/src/MessageFlagsBitField.tspackages/util/src/PermissionsBitField.tspackages/util/src/UserFlagsBitField.test.tspackages/util/src/UserFlagsBitField.tspackages/util/src/emojiShortcodes.generated.tspackages/util/src/index.ts
💤 Files with no reviewable changes (1)
- packages/util/src/index.ts
Comment on lines
185
to
195
| private _computeBasePermissions(): bigint { | ||
| let base = 0n; | ||
| const everyone = this.guild.roles.get(this.guild.id); | ||
| if (everyone) base |= BigInt(everyone.permissions); | ||
| if (everyone) base |= everyone.permissions.bitfield; | ||
| for (const roleId of this.roles.roleIds) { | ||
| if (roleId === this.guild.id) continue; | ||
| const role = this.guild.roles.get(roleId); | ||
| if (role) base |= BigInt(role.permissions); | ||
| if (role) base |= role.permissions.bitfield; | ||
| } | ||
| return base; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -e ts PermissionsBitField --exec cat -n {}Repository: fluxerjs/core
Length of output: 7709
Fix permission constants that use 2n << N instead of 1n << N, causing bit-position shifts.
The following permission flags in packages/util/src/PermissionsBitField.ts incorrectly use 2n << N as the base, which shifts the bit position one place higher than intended:
| Flag | Current (line) | Produces bit | Expected |
|---|---|---|---|
UseExternalStickers |
2n << 37n (44) |
38 | 1n << 37n |
ModerateMembers |
2n << 40n (45) |
41 | 1n << 40n |
CreateExpressions |
2n << 43n (46) |
44 | 1n << 43n |
PinMessages |
2n << 51n (47) |
52 | 1n << 51n |
BypassSlowmode |
2n << 52n (48) |
53 | 1n << 52n |
UpdateRtcRegion |
2n << 53n (49) |
54 | 1n << 53n |
Additionally, ManageEmojisAndStickers (line 42) and ManageExpressions (line 43) both define 1n << 30n — this is a duplicate alias that should be reviewed.
These errors will cause permission checks in _computeBasePermissions() and permissionsIn() to evaluate against incorrect bit positions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/fluxer-core/src/structures/GuildMember.ts` around lines 185 - 195,
The permission constants in PermissionsBitField.ts use incorrect bases like `2n
<< N` shifting bits one position; update the flagged constants
(`UseExternalStickers`, `ModerateMembers`, `CreateExpressions`, `PinMessages`,
`BypassSlowmode`, `UpdateRtcRegion`) to use `1n << N` with the correct N values
(e.g., `1n << 37n`, `1n << 40n`, etc.), and review/fix the duplicate alias where
both `ManageEmojisAndStickers` and `ManageExpressions` are defined as `1n <<
30n` (ensure each permission has the correct unique bit). This will correct bit
positions used by `_computeBasePermissions()` and `permissionsIn()`.
| if (bits in this.Flags) { | ||
| return this.Flags[bits as S]; | ||
| } | ||
| console.log(this.Flags); |
There was a problem hiding this comment.
Remove debug console.log artifacts — they'll pollute production output.
Line 38 inside resolve() logs the entire Flags dictionary on every unknown-string lookup, and Line 52 inside has() logs every single bit check. Both are debug-only and must be removed.
🔧 Proposed fix
if (typeof bits === 'string') {
if (bits in this.Flags) {
return this.Flags[bits as S];
}
- console.log(this.Flags);
// Otherwise, try to interpret the string as a valid non-negative bigint literal has(bit: BitFieldResolvable<S>): boolean {
- console.log(bit);
bit = (this.constructor as typeof BitField).resolve(bit);Also applies to: 52-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/util/src/BitField.ts` at line 38, Remove the debug console.log
statements that print internal state: delete the console.log(this.Flags) call
inside the resolve() method and the console.log used inside the has() method
(which logs each bit check); these debug prints should be removed from the
BitField class (referencing resolve(), has(), and the Flags property) so
production output is not polluted—if you need retained diagnostics, replace with
a controlled logger at a debug level instead of console.log.
Comment on lines
+40
to
+46
| // Otherwise, try to interpret the string as a valid non-negative bigint literal | ||
| try { | ||
| const value = BigInt(bits); | ||
| if (value >= 0n) return value; | ||
| } catch { | ||
| // fall through to error below | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the file structure and context around lines 40-46
echo "=== BitField.ts file content around lines 30-60 ==="
sed -n '30,60p' packages/util/src/BitField.ts
echo -e "\n=== Searching for tests with numeric string literals ==="
rg -n "resolve.*['\"][0-9]" --type ts
echo -e "\n=== Searching for BitField constructor with numeric strings ==="
rg -n "new BitField.*['\"][0-9]" --type ts
echo -e "\n=== Searching for test files in the util package ==="
find packages/util -name "*.test.ts" -o -name "*.spec.ts" | head -20Repository: fluxerjs/core
Length of output: 3403
🏁 Script executed:
#!/bin/bash
echo "=== BitField.test.ts content ==="
cat -n packages/util/src/BitField.test.ts
echo -e "\n=== PermissionsBitField.test.ts content ==="
cat -n packages/util/src/PermissionsBitField.test.ts
echo -e "\n=== Check git log for the commit message mentioned ==="
git log --oneline --all | grep -i "string inputs\|bitfield resolution" | head -5Repository: fluxerjs/core
Length of output: 6113
🏁 Script executed:
#!/bin/bash
echo "=== PermissionsBitField.ts content (first 100 lines) ==="
head -100 packages/util/src/PermissionsBitField.ts
echo -e "\n=== Search for resolvePermissionsToBitfield function ==="
rg -A 20 "function resolvePermissionsToBitfield\|const resolvePermissionsToBitfield" packages/util/src/PermissionsBitField.tsRepository: fluxerjs/core
Length of output: 4097
🏁 Script executed:
#!/bin/bash
echo "=== Recent commits affecting BitField.ts ==="
git log --oneline -20 packages/util/src/BitField.ts
echo -e "\n=== Full git diff for the relevant commit (if findable) ==="
git log --all --pretty=format:"%H %s" packages/util/src/BitField.ts | grep -i "string\|bitfield" | head -3
echo -e "\n=== Check if numeric string test exists for BitField ==="
rg -n "new.*BitField.*['\"][0-9]" packages/util/src/BitField.test.ts
rg -n "\.resolve.*['\"][0-9]" packages/util/src/BitField.test.tsRepository: fluxerjs/core
Length of output: 535
🏁 Script executed:
#!/bin/bash
echo "=== Search for commit with the exact message mentioned in review ==="
git log --all --pretty=format:"%H %s" | grep -i "string inputs\|property keys"
echo -e "\n=== Check all recent commits to BitField ==="
git log --all --oneline -30 -- packages/util/src/BitField.ts
echo -e "\n=== Look at the actual test case for invalid string ==="
sed -n '81,83p' packages/util/src/BitField.test.ts
echo -e "\n=== Verify: Does BitField.test.ts have ANY test for numeric string input? ==="
grep -n "['\"]\|[0-9]" packages/util/src/BitField.test.ts | grep -v "//"Repository: fluxerjs/core
Length of output: 2587
String literal fallback behavior lacks test coverage.
Lines 41–46 implement a BigInt(bits) fallback that accepts numeric string literals like "274877906944". However, BitField.test.ts contains no tests for this code path—it only tests property key strings ('A', 'B', 'C') and expects invalid strings to throw. If this numeric string fallback is intentional, add a test case; if unintended, remove it.
Note: The specific commit message referenced appears not to exist in the repository history. The concern stands on its own: the code permits numeric strings, but test coverage does not document or validate this behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/util/src/BitField.ts` around lines 40 - 46, The code in BitField.ts
accepts numeric string literals via BigInt(bits) (see the BigInt(bits) fallback
that returns a non-negative bigint), but tests in BitField.test.ts don't cover
this path; either add a unit test asserting that passing a numeric string like
"274877906944" yields the expected bigint/bitfield behavior, or remove the
numeric-string fallback if it's unintended. Locate the BigInt(bits) branch in
BitField.ts (the try/catch around BigInt) and either (A) add a test in
BitField.test.ts that calls the same public API (constructor/parse method that
accepts `bits`) with a numeric string and asserts the resulting value/mask, or
(B) remove the try/catch BigInt fallback so non-property strings throw as
before—update tests accordingly to reflect the chosen behavior.
Comment on lines
51
to
55
| has(bit: BitFieldResolvable<S>): boolean { | ||
| console.log(bit); | ||
| bit = (this.constructor as typeof BitField).resolve(bit); | ||
| return (this.bitfield & bit) === bit; | ||
| return (this.bitfield & bit) !== 0n; | ||
| } |
There was a problem hiding this comment.
has() uses "any-bit" semantics (!== 0n) while serialize/toArray use "all-bits" semantics (=== value) — inconsistent.
For single-bit flags (the current case) both checks are equivalent, but calling has([flagA, flagB]) resolves to a composite bigint and the current !== 0n check returns true if either bit is set. The established convention (used by serialize, toArray, and Discord.js) is to require all bits to be set.
🔧 Proposed fix (also removes the debug log)
has(bit: BitFieldResolvable<S>): boolean {
- console.log(bit);
- bit = (this.constructor as typeof BitField).resolve(bit);
- return (this.bitfield & bit) !== 0n;
+ const resolved = (this.constructor as typeof BitField).resolve(bit);
+ return (this.bitfield & resolved) === resolved;
}📝 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.
Suggested change
| has(bit: BitFieldResolvable<S>): boolean { | |
| console.log(bit); | |
| bit = (this.constructor as typeof BitField).resolve(bit); | |
| return (this.bitfield & bit) === bit; | |
| return (this.bitfield & bit) !== 0n; | |
| } | |
| has(bit: BitFieldResolvable<S>): boolean { | |
| const resolved = (this.constructor as typeof BitField).resolve(bit); | |
| return (this.bitfield & resolved) === resolved; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/util/src/BitField.ts` around lines 51 - 55, The has method currently
logs the input and uses any-bit semantics ((this.bitfield & bit) !== 0n); remove
the debug console.log and change the check to require all bits are present by
comparing equality to the resolved bit mask ((this.bitfield & bit) === bit).
Update the implementation in the BitField.has method and ensure you use
BitField.resolve (this.constructor as typeof BitField).resolve to obtain the
mask so behavior matches serialize/toArray.
Comment on lines
+44
to
+49
| UseExternalStickers : 2n << 37n, | ||
| ModerateMembers : 2n << 40n, | ||
| CreateExpressions : 2n << 43n, | ||
| PinMessages : 2n << 51n, | ||
| BypassSlowmode : 2n << 52n, | ||
| UpdateRtcRegion : 2n << 53n, |
There was a problem hiding this comment.
Critical: 2n << Xn is off-by-one — sets bit X+1, not bit X.
2n << 37n equals 2^38 (bit 38), not 2^37 (bit 37). The left operand 2n is already 2^1, so shifting it left by 37 gives 2^(1+37) = 2^38. This applies to every flag from UseExternalStickers through UpdateRtcRegion.
If the intent is bit 37, 40, 43, 51, 52, 53 respectively, use 1n << Xn:
🐛 Proposed fix
- UseExternalStickers : 2n << 37n,
- ModerateMembers : 2n << 40n,
- CreateExpressions : 2n << 43n,
- PinMessages : 2n << 51n,
- BypassSlowmode : 2n << 52n,
- UpdateRtcRegion : 2n << 53n,
+ UseExternalStickers : 1n << 37n,
+ ModerateMembers : 1n << 40n,
+ CreateExpressions : 1n << 43n,
+ PinMessages : 1n << 51n,
+ BypassSlowmode : 1n << 52n,
+ UpdateRtcRegion : 1n << 53n,The doc comment on Line 5 says "bits 31+ use 2**n" — 2n ** 37n equals 2^37 (correct), but 2n << 37n equals 2^38 (wrong). The PR description itself notes "2n ** 57n or 1n << 57n" as equivalent, confirming 1n << n is the intended form.
Please verify the intended bit positions against the Fluxer API source referenced in the comment (fluxer_api/src/constants/Channel.ts):
#!/bin/bash
# Check if the Fluxer API constants file exists and what values it defines
fd "Channel.ts" --type f | xargs grep -n -i "permission\|UseExternalStickers\|ModerateMembers\|PinMessages\|BypassSlowmode\|UpdateRtcRegion" 2>/dev/null || echo "File not found in this repo"
# Also check if there's any other reference to these bit positions
rg -n "UseExternalStickers|ModerateMembers|CreateExpressions|PinMessages|BypassSlowmode|UpdateRtcRegion" --type ts -g '!*.test.*' -g '!node_modules'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/util/src/PermissionsBitField.ts` around lines 44 - 49, The bit flags
UseExternalStickers, ModerateMembers, CreateExpressions, PinMessages,
BypassSlowmode and UpdateRtcRegion are using `2n << Xn` which sets bit X+1;
change each to `1n << Xn` (e.g. replace `2n << 37n` with `1n << 37n`) so the
flags represent the intended bits, and update/confirm the doc comment about
"bits 31+ use 2**n" remains consistent; verify the corrected values against the
Fluxer API constants (e.g. Channel.ts) to ensure they match the canonical bit
positions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
permissionsType of change
Checklist
pnpm run lint)pnpm run buildsuccessfullypnpm run testsuccessfullyFor the record, exponentiation in JS does not natively convert to bigint or negate precision issues, you must do exponentiation explicitly with bigints:
2n ** 57nor1n << 57nSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores