Add report for old NIP-51 lists#2
Open
hzrd149 wants to merge 4 commits into
Open
Conversation
WalkthroughAdds a new "Legacy Lists" diagnostic report for deprecated NIP-51 list formats, including a reactive data loader, a React report UI to migrate or delete legacy lists, registry registration, and README backlog documentation of planned reports. (50 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 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)
📝 Coding Plan
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 |
![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
🧹 Nitpick comments (1)
src/pages/reports/legacy-lists/page.tsx (1)
246-274: Consider using the filter-based class merging pattern.For consistency with the codebase style (see lines 286-289), consider using the filter pattern instead of template literals for conditional classes.
♻️ Suggested refactor
- <div className={`flex items-center gap-3 py-3 ${rowBorder}`}> + <div className={["flex items-center gap-3 py-3", rowBorder].filter(Boolean).join(" ")}>As per coding guidelines: "Use conditional class merging pattern:
['base-class', condition ? 'extra' : ''].filter(Boolean).join(' ')"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/reports/legacy-lists/page.tsx` around lines 246 - 274, The JSX branch that renders the "done" row uses a template literal for className (`className={`flex items-center gap-3 py-3 ${rowBorder}`}`) which violates the project's conditional class merging pattern; update the container and any other conditional className usages in this block (the div with rowBorder, and any other template-literal classes around the done branch) to use the filter-based pattern ['flex items-center gap-3 py-3', rowBorder].filter(Boolean).join(' ') so classes are merged consistently with the rest of the codebase (referencing the done branch, LIST_LABELS, listKey, legacyCoord, and rowBorder to locate the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 66-67: Update the NIP-05 verification CORS header check that
currently requires Access-Control-Allow-Origin: * so it instead accepts either
'*' or a specific origin; in the code/path handling the /.well-known/nostr.json
fetch and validation (the nip05 verification/CORS header validation logic),
treat a non-empty Access-Control-Allow-Origin header as valid if it equals the
requesting origin or is '*', and only flag an error when the header is missing
or empty; ensure the validation still separately verifies the pubkey match and
the optional relays field and surface each failure mode (pubkey mismatch,
unreachable endpoint, missing/invalid CORS header, relays mismatch) distinctly.
In `@src/pages/reports/legacy-lists/page.tsx`:
- Around line 154-216: The handleMerge function currently awaits publishEvent
(for modernDraft and deleteDraft), which blocks the UI; remove the await
keywords and dispatch both publishEvent(modernDraft) and
publishEvent(deleteDraft) as fire-and-forget (call them without awaiting their
Promises), then replace the immediate setDone(true); onHandled(); with a delayed
auto-advance using setTimeout(() => { setDone(true); onHandled(); }, 1500) so
the UI advances after 1500ms; apply the same non-await + 1500ms auto-advance
pattern to handleDeleteOnly as well.
- Around line 426-435: The loading branch using the isLoading check currently
returns only a spinner and text; modify that return (the JSX inside the if
(isLoading) block in page.tsx) to also render the standard "Skip" button
component used on report pages (keep the spinner and text), ensuring the Skip
button is visible from the first frame; locate the isLoading conditional and add
the same Skip button element (props/handler consistent with other report pages)
so keyboard and click behavior match existing report Skip behavior.
- Around line 398-404: The effect currently calls onDone immediately when
isClean is true; change the useEffect in the component that references isClean,
doneFired, and onDone to schedule the onDone({ status: "clean", summary: "No
legacy NIP-51 lists found" }) call using setTimeout with a 1500ms delay, store
the timer id, set doneFired.current = true when scheduling (or right before
invoking) and return a cleanup that clears the timeout to avoid leaks; keep the
dependency array as [isClean].
- Around line 407-416: When allHandled becomes true the effect should wait
1500ms then call onDone instead of firing immediately; modify the useEffect that
references allHandled/doneFired to start a timeout (1500ms) which calls onDone({
status: "fixed", summary: `${foundCount} legacy list${foundCount !== 1 ? "s" :
""} removed` }) and set doneFired.current = true inside that timeout, and return
a cleanup that clears the timeout to avoid leaks/cancel the auto-advance; keep
the doneFired guard so the callback only schedules once.
---
Nitpick comments:
In `@src/pages/reports/legacy-lists/page.tsx`:
- Around line 246-274: The JSX branch that renders the "done" row uses a
template literal for className (`className={`flex items-center gap-3 py-3
${rowBorder}`}`) which violates the project's conditional class merging pattern;
update the container and any other conditional className usages in this block
(the div with rowBorder, and any other template-literal classes around the done
branch) to use the filter-based pattern ['flex items-center gap-3 py-3',
rowBorder].filter(Boolean).join(' ') so classes are merged consistently with the
rest of the codebase (referencing the done branch, LIST_LABELS, listKey,
legacyCoord, and rowBorder to locate the code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33c66c87-fb77-48ff-9e4d-9d07263fbd27
📒 Files selected for processing (4)
README.mdsrc/pages/reports/legacy-lists/loader.tssrc/pages/reports/legacy-lists/page.tsxsrc/pages/reports/sections.tsx
Comment on lines
+66
to
+67
| Fetch the user's `nip05` field from their kind:0 profile, hit the `/.well-known/nostr.json` endpoint, and verify: (a) the pubkey in the response matches the user's pubkey, (b) the endpoint is reachable with correct CORS headers (`Access-Control-Allow-Origin: *`), (c) the optional `relays` field in the response, if present, aligns with their NIP-65 relay list. Surface each failure mode clearly so the user knows exactly what is broken. | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For browser fetch/CORS validation, is Access-Control-Allow-Origin: * strictly required, or is a specific allowed origin also valid? Please confirm with MDN CORS docs and any NIP-05 implementation guidance.
💡 Result:
No—Access-Control-Allow-Origin: * is not strictly required for browser fetch()/CORS to succeed.
- MDN (CORS) says
Access-Control-Allow-Origincan be either a single specific origin or*:
Access-Control-Allow-Origin: <origin> | *and a server may reply with e.g.Access-Control-Allow-Origin: https://mozilla.orgto allow only that origin. (developer.mozilla.org)
MDN also explicitly shows restricting access by returning a specific allowed origin (e.g.https://foo.example). (developer.mozilla.org) - Credentials caveat (MDN): if the request is credentialed (cookies/auth), the server must not use
*; it must return an explicit origin (and*will be blocked by browsers in that case). (developer.mozilla.org) - NIP-05 guidance: the NIP-05 documentation/implementations commonly recommend
Access-Control-Allow-Origin: *so any web-based Nostr client (running on arbitrary origins) can validate/.well-known/nostr.json. (nostr-nips.com)
So:
- For general CORS: specific origin is valid (and often preferable). (developer.mozilla.org)
- For NIP-05 specifically:
*is recommended for broad compatibility with “pure JS” browser clients. (nostr-nips.com)
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/CORS
- 2: https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/CORS
- 3: https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/CORS
- 4: https://www.nostr-nips.com/nip-05
- 5: https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/CORS
- 6: https://www.nostr-nips.com/nip-05
Relax the CORS header check to avoid false negatives.
Line 66 requires Access-Control-Allow-Origin: *, but valid deployments may return a specific origin (e.g., Access-Control-Allow-Origin: https://example.com) and still be fully browser-accessible. The CORS specification permits either form, and checking only for * will incorrectly flag healthy NIP-05 endpoints as broken. Accept both * and specific allowed origins.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 66 - 67, Update the NIP-05 verification CORS header
check that currently requires Access-Control-Allow-Origin: * so it instead
accepts either '*' or a specific origin; in the code/path handling the
/.well-known/nostr.json fetch and validation (the nip05 verification/CORS header
validation logic), treat a non-empty Access-Control-Allow-Origin header as valid
if it equals the requesting origin or is '*', and only flag an error when the
header is missing or empty; ensure the validation still separately verifies the
pubkey match and the optional relays field and surface each failure mode (pubkey
mismatch, unreachable endpoint, missing/invalid CORS header, relays mismatch)
distinctly.
Comment on lines
+154
to
+216
| async function handleMerge() { | ||
| if (!event || merging) return; | ||
| setMerging(true); | ||
| setError(null); | ||
| try { | ||
| const tagsToMigrate = collectMigrateTags(); | ||
| const modernKind = MODERN_KIND[listKey]; | ||
|
|
||
| // Fetch the current modern list from the store (may be undefined if it | ||
| // doesn't exist yet). We read only public tags from it to avoid | ||
| // accidentally discarding existing hidden content. | ||
| const modernEvent: NostrEvent | undefined = eventStore.getReplaceable( | ||
| modernKind, | ||
| pubkey, | ||
| ); | ||
|
|
||
| const existingPublicTags = modernEvent | ||
| ? getListTags(modernEvent, "public").filter( | ||
| ([n]: string[]) => n !== "d", | ||
| ) | ||
| : []; | ||
|
|
||
| // Merge and deduplicate by serialised tag value | ||
| const seen = new Set<string>( | ||
| existingPublicTags.map((t) => JSON.stringify(t)), | ||
| ); | ||
| const newTags: string[][] = [...existingPublicTags]; | ||
| for (const tag of tagsToMigrate) { | ||
| const key = JSON.stringify(tag); | ||
| if (!seen.has(key)) { | ||
| seen.add(key); | ||
| newTags.push(tag); | ||
| } | ||
| } | ||
|
|
||
| // 1. Publish updated modern list | ||
| const modernDraft = modernEvent | ||
| ? await factory.modify( | ||
| modernEvent, | ||
| modifyPublicTags(() => newTags), | ||
| ) | ||
| : await factory.build( | ||
| { kind: modernKind }, | ||
| modifyPublicTags(() => newTags), | ||
| ); | ||
| await publishEvent(modernDraft); | ||
|
|
||
| // 2. Publish kind:5 deletion event — trust relays to remove the legacy list | ||
| const coord = legacyCoordinate(listKey, pubkey); | ||
| const deleteDraft = await factory.build({ | ||
| kind: 5, | ||
| tags: [["a", coord]], | ||
| }); | ||
| await publishEvent(deleteDraft); | ||
|
|
||
| setDone(true); | ||
| onHandled(); | ||
| } catch (e) { | ||
| setError(e instanceof Error ? e.message : "Failed to merge list."); | ||
| } finally { | ||
| setMerging(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid awaiting publish() calls — dispatch fire-and-forget.
The handleMerge function awaits both publishEvent calls (lines 199 and 207). Per coding guidelines, publish calls should be fire-and-forget to avoid blocking the UI, then auto-advance after 1500ms.
🛠️ Proposed fix
async function handleMerge() {
if (!event || merging) return;
setMerging(true);
setError(null);
try {
const tagsToMigrate = collectMigrateTags();
const modernKind = MODERN_KIND[listKey];
// ... existing merge logic ...
// 1. Publish updated modern list
const modernDraft = modernEvent
? await factory.modify(
modernEvent,
modifyPublicTags(() => newTags),
)
: await factory.build(
{ kind: modernKind },
modifyPublicTags(() => newTags),
);
- await publishEvent(modernDraft);
+ publishEvent(modernDraft);
// 2. Publish kind:5 deletion event
const coord = legacyCoordinate(listKey, pubkey);
const deleteDraft = await factory.build({
kind: 5,
tags: [["a", coord]],
});
- await publishEvent(deleteDraft);
+ publishEvent(deleteDraft);
setDone(true);
onHandled();
} catch (e) {
setError(e instanceof Error ? e.message : "Failed to merge list.");
} finally {
setMerging(false);
}
}Apply the same pattern to handleDeleteOnly (line 229).
As per coding guidelines: "Do not await or serialize publish() calls. Dispatch all needed publishes fire-and-forget."
📝 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
| async function handleMerge() { | |
| if (!event || merging) return; | |
| setMerging(true); | |
| setError(null); | |
| try { | |
| const tagsToMigrate = collectMigrateTags(); | |
| const modernKind = MODERN_KIND[listKey]; | |
| // Fetch the current modern list from the store (may be undefined if it | |
| // doesn't exist yet). We read only public tags from it to avoid | |
| // accidentally discarding existing hidden content. | |
| const modernEvent: NostrEvent | undefined = eventStore.getReplaceable( | |
| modernKind, | |
| pubkey, | |
| ); | |
| const existingPublicTags = modernEvent | |
| ? getListTags(modernEvent, "public").filter( | |
| ([n]: string[]) => n !== "d", | |
| ) | |
| : []; | |
| // Merge and deduplicate by serialised tag value | |
| const seen = new Set<string>( | |
| existingPublicTags.map((t) => JSON.stringify(t)), | |
| ); | |
| const newTags: string[][] = [...existingPublicTags]; | |
| for (const tag of tagsToMigrate) { | |
| const key = JSON.stringify(tag); | |
| if (!seen.has(key)) { | |
| seen.add(key); | |
| newTags.push(tag); | |
| } | |
| } | |
| // 1. Publish updated modern list | |
| const modernDraft = modernEvent | |
| ? await factory.modify( | |
| modernEvent, | |
| modifyPublicTags(() => newTags), | |
| ) | |
| : await factory.build( | |
| { kind: modernKind }, | |
| modifyPublicTags(() => newTags), | |
| ); | |
| await publishEvent(modernDraft); | |
| // 2. Publish kind:5 deletion event — trust relays to remove the legacy list | |
| const coord = legacyCoordinate(listKey, pubkey); | |
| const deleteDraft = await factory.build({ | |
| kind: 5, | |
| tags: [["a", coord]], | |
| }); | |
| await publishEvent(deleteDraft); | |
| setDone(true); | |
| onHandled(); | |
| } catch (e) { | |
| setError(e instanceof Error ? e.message : "Failed to merge list."); | |
| } finally { | |
| setMerging(false); | |
| } | |
| } | |
| async function handleMerge() { | |
| if (!event || merging) return; | |
| setMerging(true); | |
| setError(null); | |
| try { | |
| const tagsToMigrate = collectMigrateTags(); | |
| const modernKind = MODERN_KIND[listKey]; | |
| // Fetch the current modern list from the store (may be undefined if it | |
| // doesn't exist yet). We read only public tags from it to avoid | |
| // accidentally discarding existing hidden content. | |
| const modernEvent: NostrEvent | undefined = eventStore.getReplaceable( | |
| modernKind, | |
| pubkey, | |
| ); | |
| const existingPublicTags = modernEvent | |
| ? getListTags(modernEvent, "public").filter( | |
| ([n]: string[]) => n !== "d", | |
| ) | |
| : []; | |
| // Merge and deduplicate by serialised tag value | |
| const seen = new Set<string>( | |
| existingPublicTags.map((t) => JSON.stringify(t)), | |
| ); | |
| const newTags: string[][] = [...existingPublicTags]; | |
| for (const tag of tagsToMigrate) { | |
| const key = JSON.stringify(tag); | |
| if (!seen.has(key)) { | |
| seen.add(key); | |
| newTags.push(tag); | |
| } | |
| } | |
| // 1. Publish updated modern list | |
| const modernDraft = modernEvent | |
| ? await factory.modify( | |
| modernEvent, | |
| modifyPublicTags(() => newTags), | |
| ) | |
| : await factory.build( | |
| { kind: modernKind }, | |
| modifyPublicTags(() => newTags), | |
| ); | |
| publishEvent(modernDraft); | |
| // 2. Publish kind:5 deletion event — trust relays to remove the legacy list | |
| const coord = legacyCoordinate(listKey, pubkey); | |
| const deleteDraft = await factory.build({ | |
| kind: 5, | |
| tags: [["a", coord]], | |
| }); | |
| publishEvent(deleteDraft); | |
| setDone(true); | |
| onHandled(); | |
| } catch (e) { | |
| setError(e instanceof Error ? e.message : "Failed to merge list."); | |
| } finally { | |
| setMerging(false); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/reports/legacy-lists/page.tsx` around lines 154 - 216, The
handleMerge function currently awaits publishEvent (for modernDraft and
deleteDraft), which blocks the UI; remove the await keywords and dispatch both
publishEvent(modernDraft) and publishEvent(deleteDraft) as fire-and-forget (call
them without awaiting their Promises), then replace the immediate setDone(true);
onHandled(); with a delayed auto-advance using setTimeout(() => { setDone(true);
onHandled(); }, 1500) so the UI advances after 1500ms; apply the same non-await
+ 1500ms auto-advance pattern to handleDeleteOnly as well.
Comment on lines
+407
to
+416
| useEffect(() => { | ||
| if (allHandled && !doneFired.current) { | ||
| doneFired.current = true; | ||
| onDone({ | ||
| status: "fixed", | ||
| summary: `${foundCount} legacy list${foundCount !== 1 ? "s" : ""} removed`, | ||
| }); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [allHandled]); |
There was a problem hiding this comment.
Add 1500ms auto-advance after all lists handled.
Similar to the clean state, when all found lists have been handled, the page should auto-advance after 1500ms with a cleanup function.
🛠️ Proposed fix
useEffect(() => {
if (allHandled && !doneFired.current) {
doneFired.current = true;
onDone({
status: "fixed",
summary: `${foundCount} legacy list${foundCount !== 1 ? "s" : ""} removed`,
});
+ const t = setTimeout(() => onContinue(), 1500);
+ return () => clearTimeout(t);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
- }, [allHandled]);
+ }, [allHandled, onContinue]);📝 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
| useEffect(() => { | |
| if (allHandled && !doneFired.current) { | |
| doneFired.current = true; | |
| onDone({ | |
| status: "fixed", | |
| summary: `${foundCount} legacy list${foundCount !== 1 ? "s" : ""} removed`, | |
| }); | |
| } | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [allHandled]); | |
| useEffect(() => { | |
| if (allHandled && !doneFired.current) { | |
| doneFired.current = true; | |
| onDone({ | |
| status: "fixed", | |
| summary: `${foundCount} legacy list${foundCount !== 1 ? "s" : ""} removed`, | |
| }); | |
| const t = setTimeout(() => onContinue(), 1500); | |
| return () => clearTimeout(t); | |
| } | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [allHandled, onContinue]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/reports/legacy-lists/page.tsx` around lines 407 - 416, When
allHandled becomes true the effect should wait 1500ms then call onDone instead
of firing immediately; modify the useEffect that references allHandled/doneFired
to start a timeout (1500ms) which calls onDone({ status: "fixed", summary:
`${foundCount} legacy list${foundCount !== 1 ? "s" : ""} removed` }) and set
doneFired.current = true inside that timeout, and return a cleanup that clears
the timeout to avoid leaks/cancel the auto-advance; keep the doneFired guard so
the callback only schedules once.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pages/reports/legacy-lists/page.tsx (1)
397-419:⚠️ Potential issue | 🟠 MajorAuto-advance flow is still incomplete and timing guards are brittle.
Line 401 only schedules
onDone(notonContinue), and Line 400 setsdoneFired.current = truebefore the timeout callback. Also, Line 413 triggers completion immediately forallHandledinstead of the required delayed auto-advance. Please move the guard mutation into the timeout callback, use a shared timeout constant fromlib/timeouts.ts, and auto-continue after reporting completion.🛠️ Proposed fix
+import { REPORT_AUTO_ADVANCE_MS } from "../../../lib/timeouts.ts"; useEffect(() => { - if (isClean && !doneFired.current) { - doneFired.current = true; - const timer = setTimeout(() => { - onDone({ status: "clean", summary: "No legacy NIP-51 lists found" }); - }, 1500); - return () => clearTimeout(timer); - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [isClean]); + if (!isClean || doneFired.current) return; + const timer = setTimeout(() => { + if (doneFired.current) return; + doneFired.current = true; + onDone({ status: "clean", summary: "No legacy NIP-51 lists found" }); + onContinue(); + }, REPORT_AUTO_ADVANCE_MS); + return () => clearTimeout(timer); + }, [isClean, onDone, onContinue]); useEffect(() => { - if (allHandled && !doneFired.current) { - doneFired.current = true; - onDone({ - status: "fixed", - summary: `${foundCount} legacy list${foundCount !== 1 ? "s" : ""} removed`, - }); - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [allHandled]); + if (!allHandled || doneFired.current) return; + const timer = setTimeout(() => { + if (doneFired.current) return; + doneFired.current = true; + onDone({ + status: "fixed", + summary: `${foundCount} legacy list${foundCount !== 1 ? "s" : ""} removed`, + }); + onContinue(); + }, REPORT_AUTO_ADVANCE_MS); + return () => clearTimeout(timer); + }, [allHandled, foundCount, onDone, onContinue]);As per coding guidelines: "Report pages with no issues found must auto-advance to the next report after 1500 ms using
setTimeoutinsideuseEffect." and "Import timeout constants fromlib/timeouts.tsinstead of declaring local timeout constants in report pages."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/reports/legacy-lists/page.tsx` around lines 397 - 419, The auto-advance logic is brittle: move the mutation of doneFired.current into the timeout callback (do not set it before scheduling), use the shared timeout constant imported from lib/timeouts.ts (e.g. AUTO_ADVANCE_MS) instead of a local literal, and ensure both branches (isClean and allHandled) schedule a delayed callback that first calls onDone(...) then calls onContinue() to advance; update the useEffect for isClean to schedule setTimeout(() => { doneFired.current = true; onDone(...); onContinue(); }, AUTO_ADVANCE_MS) and change the allHandled useEffect to do the same (use foundCount when building the summary), keeping the same dependency arrays and retaining the guard checks for !doneFired.current.
🧹 Nitpick comments (1)
src/pages/reports/legacy-lists/page.tsx (1)
244-289: Use the required conditional class merge pattern consistently.Line 249, Line 277, and Line 286-Line 289 currently use template/interpolated class composition. Please switch to the mandated array +
filter(Boolean).join(' ')pattern for conditional classes.As per coding guidelines: "Use conditional class merging pattern:
['base-class', condition ? 'extra' : ''].filter(Boolean).join(' ')instead of ternaries or inline concatenation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/reports/legacy-lists/page.tsx` around lines 244 - 289, Replace template-string/interpolated className usage with the mandated conditional array + filter(Boolean).join(' ') pattern: change className={`flex items-center gap-3 py-3 ${rowBorder}`} and className={`py-3 flex flex-col gap-2 ${rowBorder}`} (and any other template concatenations around lines that build row classes) to className={['flex items-center gap-3 py-3', rowBorder].filter(Boolean).join(' ')} and className={['py-3 flex flex-col gap-2', rowBorder].filter(Boolean).join(' ')} respectively; likewise convert any other interpolated class uses around the LIST_LABELS[listKey] / entry.hasHidden / effectivelyUnlocked spans to the same array + filter(Boolean).join(' ') form so all conditional classes follow the required pattern.
🤖 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/pages/reports/legacy-lists/page.tsx`:
- Around line 73-78: The ReadOnlyBanner component currently includes an inline
sign-in prompt ("sign in as this account to publish them"); remove that phrase
so the banner remains informational only (e.g., "You're viewing someone else's
account. Actions will be queued as drafts and surfaced later.")—update the
ReadOnlyBanner return string accordingly and ensure no "sign in" or
publish-instruction wording remains in ReadOnlyBanner or related JSX in this
file so draft surfacing/sign-in flows remain handled by the complete/ step.
---
Duplicate comments:
In `@src/pages/reports/legacy-lists/page.tsx`:
- Around line 397-419: The auto-advance logic is brittle: move the mutation of
doneFired.current into the timeout callback (do not set it before scheduling),
use the shared timeout constant imported from lib/timeouts.ts (e.g.
AUTO_ADVANCE_MS) instead of a local literal, and ensure both branches (isClean
and allHandled) schedule a delayed callback that first calls onDone(...) then
calls onContinue() to advance; update the useEffect for isClean to schedule
setTimeout(() => { doneFired.current = true; onDone(...); onContinue(); },
AUTO_ADVANCE_MS) and change the allHandled useEffect to do the same (use
foundCount when building the summary), keeping the same dependency arrays and
retaining the guard checks for !doneFired.current.
---
Nitpick comments:
In `@src/pages/reports/legacy-lists/page.tsx`:
- Around line 244-289: Replace template-string/interpolated className usage with
the mandated conditional array + filter(Boolean).join(' ') pattern: change
className={`flex items-center gap-3 py-3 ${rowBorder}`} and className={`py-3
flex flex-col gap-2 ${rowBorder}`} (and any other template concatenations around
lines that build row classes) to className={['flex items-center gap-3 py-3',
rowBorder].filter(Boolean).join(' ')} and className={['py-3 flex flex-col
gap-2', rowBorder].filter(Boolean).join(' ')} respectively; likewise convert any
other interpolated class uses around the LIST_LABELS[listKey] / entry.hasHidden
/ effectivelyUnlocked spans to the same array + filter(Boolean).join(' ') form
so all conditional classes follow the required pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c20a5287-9027-4d91-b629-65fb22a6c0fe
📒 Files selected for processing (1)
src/pages/reports/legacy-lists/page.tsx
Comment on lines
+73
to
+78
| function ReadOnlyBanner() { | ||
| return ( | ||
| <div className="bg-info/10 border border-info/30 rounded-xl p-3 text-xs text-info"> | ||
| You're viewing someone else's account. Actions will be queued as drafts — | ||
| sign in as this account to publish them. | ||
| </div> |
There was a problem hiding this comment.
Remove inline sign-in prompt wording from the report card.
Line 76-Line 77 explicitly prompts sign-in inside this report page. This should be delegated to the complete/ step; keep this banner informational only (e.g., drafts queued and surfaced later).
Based on learnings: "Applies to **/pages/reports/*/page.tsx : Report pages must not add inline sign-in prompts. The complete/ page handles draft surfacing and sign-in prompts."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/reports/legacy-lists/page.tsx` around lines 73 - 78, The
ReadOnlyBanner component currently includes an inline sign-in prompt ("sign in
as this account to publish them"); remove that phrase so the banner remains
informational only (e.g., "You're viewing someone else's account. Actions will
be queued as drafts and surfaced later.")—update the ReadOnlyBanner return
string accordingly and ensure no "sign in" or publish-instruction wording
remains in ReadOnlyBanner or related JSX in this file so draft surfacing/sign-in
flows remain handled by the complete/ step.
erskingardner
left a comment
erskingardner
left a comment
Member
There was a problem hiding this comment.
THe agent picked up a few things
| // Fetch the current modern list from the store (may be undefined if it | ||
| // doesn't exist yet). We read only public tags from it to avoid | ||
| // accidentally discarding existing hidden content. | ||
| const modernEvent: NostrEvent | undefined = eventStore.getReplaceable( |
Member
There was a problem hiding this comment.
If the user already has a kind:10000/10001/10003/10004 event that hasn't been loaded into the local EventStore yet, getReplaceable() returns undefined here and the code falls back to building a brand-new replacement list from only the legacy tags. Because this report never loads those standard-list kinds first, that case is common, and publishing the new replaceable event will overwrite the user's existing modern list on relays, dropping any items that were only present in the current standard list.
| ); | ||
| if (!effectivelyUnlocked) return pub; | ||
| const hidden = | ||
| getHiddenTags(event)?.filter(([n]: string[]) => n !== "d") ?? []; |
Member
There was a problem hiding this comment.
If a legacy list contains encrypted items and the user clicks Unlock hidden items, this appends those hidden tags into the same array that is later written with modifyPublicTags(). In that case the merge republishes formerly private mutes/bookmarks/communities as public tags on the new standard list instead of preserving them in encrypted content, which leaks data the user had intentionally hidden.
| function collectMigrateTags(): string[][] { | ||
| if (!event) return []; | ||
| const pub = getListTags(event, "public").filter( | ||
| ([n]: string[]) => n !== "d", |
Member
There was a problem hiding this comment.
NIP-51 addressable sets can carry non-item metadata such as title, image, and description; filtering out only d means those tags are treated as migrateable items. On any legacy set with that metadata, Merge will republish those bookkeeping tags into kind:10000/10001/10003/10004 and can even show a non-zero item count when the set has no real list entries.
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.
Summary by CodeRabbit
New Features
Documentation