Serialize make-me-rich settings updates without eager refresh#1518
Serialize make-me-rich settings updates without eager refresh#1518KillariDev wants to merge 3 commits into
Conversation
|
/review |
KillariDev
force-pushed
the
t3code/83482476
branch
from
72a2b65 to
eee8bef
Compare
KillariDev
changed the title
|
/review |
1 similar comment
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
CI Agent Review
The hand-rolled element-by-element equality check in updateFixedMakeMeRichList is a maintainability concern: if RichListElement gains a new field in the future, the comparison will not account for it, causing the function to incorrectly report no change and skip the storage write.
| return await makeMeRichSettingsSemaphore.execute(async () => { | ||
| const previous = await getFixedAddressRichList() | ||
| const next = update(previous) | ||
| if (previous.length === next.length && previous.every((element, index) => { |
There was a problem hiding this comment.
Maintainability concern: the hand-rolled element-by-element equality check only compares address, makingRich, and type. If RichListElement gains a new field, this comparison must be manually updated — otherwise a real change in the new field would be missed, causing the function to return false (no change) and skip the storage write. Consider a more robust comparison approach that automatically covers all fields.
KillariDev
force-pushed
the
t3code/83482476
branch
from
eee8bef to
6a973dd
Compare
KillariDev
changed the title
KillariDev
force-pushed
the
t3code/83482476
branch
8 times, most recently
from
6ea82dd to
861e022
Compare
- add atomic settings updates for rich-list state - preserve concurrent user edits while tracking previous active address - make repeated rich-address adds idempotent without reordering entries - avoid full popup visualization refreshes when rich mode changes - keep new rich balance overrides visible to the next simulation input
KillariDev
force-pushed
the
t3code/83482476
branch
from
861e022 to
6e8dc06
Compare
KillariDev
changed the title
|
/review |
There was a problem hiding this comment.
CI Agent Review
The equality comparison infrastructure introduced in settings.ts is over-abstracted and fragile. The ComparableRichListElement mapped type is structurally identical to RichListElement (an identity type adding indirection with no benefit), and the comparableRichListElement function manually copies only the three current fields of RichListElement. When RichListElement (defined as a funtypes.ReadonlyObject schema in storageUtils.ts) gains a new field in the future, this manual copy will silently drop it, causing richListElementsEqual to incorrectly consider two elements equal. This has a real consequence: updateFixedMakeMeRichList uses richListElementsEqual to decide whether to skip writing to storage — a silently incorrect equality result would skip a necessary storage write, leaving persisted state stale and difficult to trace. Consider simplifying to direct field comparison or reusing the existing arePropValuesEqual utility from typed-arrays.ts (which would need to be exported first).
| } | ||
|
|
||
| function comparableRichListElement(element: RichListElement): ComparableRichListElement { | ||
| return { |
There was a problem hiding this comment.
The comparableRichListElement function manually copies three fields (address, makingRich, type) from RichListElement. Since RichListElement is defined as a funtypes.ReadonlyObject schema in storageUtils.ts, adding a new field to that schema in the future would cause this function to silently drop it. richListElementsEqual (line 181) would then incorrectly consider two elements equal even when they differ in the new field. This has a real consequence: updateFixedMakeMeRichList (line 187) uses richListElementsEqual to decide whether to skip writing to storage (return false means no write), so a silently incorrect equality result would skip a necessary storage write, leaving persisted state stale. The ComparableRichListElement mapped type (line 169) is also an identity type — [Key in keyof RichListElement]: RichListElement[Key] produces a type structurally identical to RichListElement, adding indirection with no benefit. Consider direct field comparison (first.address === second.address && first.makingRich === second.makingRich && first.type === second.type) or reusing the existing arePropValuesEqual utility from typed-arrays.ts (currently not exported).
# Conflicts: # app/ts/background/popupMessageHandlers.ts # app/ts/components/pages/Home.tsx
KillariDev
changed the title
|
/review |
|
|
||
| type ComparableRichListElement = { | ||
| [Key in keyof RichListElement]: RichListElement[Key] | ||
| } |
There was a problem hiding this comment.
The ComparableRichListElement mapped type is structurally identical to RichListElement ({ [Key in keyof RichListElement]: RichListElement[Key] } produces the same type), and comparableRichListElement reconstructs an object with the same three fields. This adds a type, a function, and an indirection layer for zero architectural benefit. Additionally, richListElementsEqual relies on Object.values enumeration order for correctness — while stable in V8 for string-keyed properties, it is not guaranteed by the type system or the RichListElement schema. If RichListElement gains a new field, this function will silently compare it with ===, failing for non-primitive types. A direct field-by-field comparison would be explicit and resilient to schema evolution. This also introduces a third change-detection strategy (deep comparison via Object.values) alongside the existing reference-equality approach used in updateWebsiteAccess, growing inconsistency in the semaphore-protected-update pattern.
|
/review |
1 participant
Summary
mainand preserve its initial-load safeguards for rich-list controls.Validation
bun test— 618 passed, 0 failedbun run setup-chromebun run typecheckbun run lint