Add article deletion support and 404 page#224
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances article management and user experience by introducing article deletion capabilities and a proper 404 page for missing content. It also provides a dedicated article editing interface and improves the note composition experience by allowing users to easily quote existing posts. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality, including article deletion, a 404 page for missing articles, and an article editing page. It also adds the ability to quote posts. The changes are extensive, touching both the GraphQL backend and the SolidJS frontend. The code is generally well-structured, with good separation of concerns (e.g., moving post lookup logic to a dedicated lookup.ts file). My review focuses on a few areas for improvement regarding performance, error handling, and code duplication in the frontend.
| } catch { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The catch block is empty. While returning null is the correct behavior for a failed lookup, it's good practice to log the error. This will be very helpful for debugging federation issues in the future. The repository style guide (lines 74-75) recommends using LogTape for structured logging.
| } catch { | |
| return null; | |
| } | |
| } catch (error) { | |
| // TODO: Use the app's logger instance if available in the context. | |
| console.error(`Failed to lookup remote object at ${url}`, error); | |
| return null; | |
| } |
References
- The style guide requires using structured logging via LogTape and including context in error details. The current empty catch block does not log any error information. (link)
| toc: t.field({ | ||
| type: "JSON", | ||
| description: "Table of contents for the article content.", | ||
| select: { | ||
| columns: { content: true }, | ||
| }, | ||
| async resolve(content, _, ctx) { | ||
| const rendered = await renderMarkup(ctx.fedCtx, content.content, { | ||
| kv: ctx.kv, | ||
| }); | ||
| return rendered.toc; | ||
| }, | ||
| }), |
There was a problem hiding this comment.
The renderMarkup function is called here to get the table of contents, and it's also called in the content field resolver on lines 356-361. Since renderMarkup can be an expensive operation, calling it twice for the same content is inefficient. Consider caching the result of renderMarkup for the duration of the request to avoid redundant processing. You could potentially attach the result to the content object after the first call and reuse it.
| <Show | ||
| when={article().actor.local} | ||
| fallback={ | ||
| <a | ||
| href={article().contents?.[0]?.url ?? article().url ?? | ||
| article().iri} | ||
| lang={article().contents?.[0]?.language ?? | ||
| article().language ?? undefined} | ||
| hreflang={article().contents?.[0]?.language ?? | ||
| article().language ?? undefined} | ||
| target="_blank" | ||
| on:mouseover={() => props.setHover?.(true)} | ||
| on:mouseout={() => props.setHover?.(false)} | ||
| class="block p-4" | ||
| > | ||
| {article().contents?.[0]?.title ?? article().name} | ||
| </a> | ||
| } | ||
| > | ||
| {article().contents?.[0]?.title ?? article().name} | ||
| </a> | ||
| <InternalLink | ||
| href={article().contents?.[0]?.url ?? article().url ?? | ||
| article().iri} | ||
| internalHref={`/@${article().actor.username}/${article().publishedYear}/${article().slug}`} | ||
| lang={article().contents?.[0]?.language ?? | ||
| article().language ?? undefined} | ||
| hreflang={article().contents?.[0]?.language ?? | ||
| article().language ?? undefined} | ||
| on:mouseover={() => props.setHover?.(true)} | ||
| on:mouseout={() => props.setHover?.(false)} | ||
| class="block p-4" | ||
| > | ||
| {article().contents?.[0]?.title ?? article().name} | ||
| </InternalLink> | ||
| </Show> | ||
| </h1> | ||
| </Show> | ||
| <Show | ||
| when={article().contents?.[0]?.summary ?? article().summary} | ||
| fallback={ | ||
| <a | ||
| href={article().url ?? article().iri} | ||
| lang={article().language ?? undefined} | ||
| hreflang={article().language ?? undefined} | ||
| target={article().contents?.[0]?.url == null | ||
| ? "_blank" | ||
| : undefined} | ||
| on:mouseover={() => props.setHover?.(true)} | ||
| on:mouseout={() => props.setHover?.(false)} | ||
| class="px-4 pb-4" | ||
| <Show | ||
| when={article().actor.local} | ||
| fallback={ | ||
| <a | ||
| href={article().url ?? article().iri} | ||
| lang={article().language ?? undefined} | ||
| hreflang={article().language ?? undefined} | ||
| target="_blank" | ||
| on:mouseover={() => props.setHover?.(true)} | ||
| on:mouseout={() => props.setHover?.(false)} | ||
| class="px-4 pb-4" | ||
| > | ||
| <div | ||
| innerHTML={article().content} | ||
| class="line-clamp-4 overflow-hidden" | ||
| /> | ||
| </a> | ||
| } | ||
| > | ||
| <div | ||
| innerHTML={article().content} | ||
| class="line-clamp-4 overflow-hidden" | ||
| /> | ||
| </a> | ||
| <InternalLink | ||
| href={article().url ?? article().iri} | ||
| internalHref={`/@${article().actor.username}/${article().publishedYear}/${article().slug}`} | ||
| lang={article().language ?? undefined} | ||
| hreflang={article().language ?? undefined} | ||
| on:mouseover={() => props.setHover?.(true)} | ||
| on:mouseout={() => props.setHover?.(false)} | ||
| class="px-4 pb-4" | ||
| > | ||
| <div | ||
| innerHTML={article().content} | ||
| class="line-clamp-4 overflow-hidden" | ||
| /> | ||
| </InternalLink> | ||
| </Show> | ||
| } | ||
| > | ||
| {(summary) => ( | ||
| <Show | ||
| when={article().actor.local} | ||
| fallback={ | ||
| <a | ||
| href={article().contents?.[0]?.url ?? article().url ?? | ||
| article().iri} | ||
| innerHTML={summary()} | ||
| lang={article().contents?.[0]?.language ?? | ||
| article().language ?? undefined} | ||
| hreflang={article().contents?.[0]?.language ?? | ||
| article().language ?? undefined} | ||
| target="_blank" | ||
| on:mouseover={() => props.setHover?.(true)} | ||
| on:mouseout={() => props.setHover?.(false)} | ||
| data-llm-summary-label={t`Summarized by LLM`} | ||
| class="prose dark:prose-invert break-words overflow-wrap px-4 pb-4 before:content-[attr(data-llm-summary-label)] before:mr-1 before:text-sm before:bg-muted before:text-muted-foreground before:p-1 before:rounded-sm before:border" | ||
| classList={{ | ||
| "before:border-transparent": !props.hover?.(), | ||
| }} | ||
| /> | ||
| } | ||
| > | ||
| <InternalLink | ||
| href={article().contents?.[0]?.url ?? article().url ?? | ||
| article().iri} | ||
| internalHref={`/@${article().actor.username}/${article().publishedYear}/${article().slug}`} | ||
| innerHTML={summary()} | ||
| lang={article().contents?.[0]?.language ?? | ||
| article().language ?? undefined} | ||
| hreflang={article().contents?.[0]?.language ?? | ||
| article().language ?? undefined} | ||
| on:mouseover={() => props.setHover?.(true)} | ||
| on:mouseout={() => props.setHover?.(false)} | ||
| data-llm-summary-label={t`Summarized by LLM`} | ||
| class="prose dark:prose-invert break-words overflow-wrap px-4 pb-4 before:content-[attr(data-llm-summary-label)] before:mr-1 before:text-sm before:bg-muted before:text-muted-foreground before:p-1 before:rounded-sm before:border" | ||
| classList={{ | ||
| "before:border-transparent": !props.hover?.(), | ||
| }} | ||
| /> | ||
| </Show> | ||
| )} | ||
| </Show> | ||
| <Show | ||
| when={article().actor.local} | ||
| fallback={ | ||
| <a | ||
| href={article().contents?.[0]?.url ?? article().url ?? | ||
| article().iri} | ||
| innerHTML={summary()} | ||
| lang={article().contents?.[0]?.language ?? article().language ?? | ||
| undefined} | ||
| hreflang={article().contents?.[0]?.language ?? | ||
| article().language ?? | ||
| undefined} | ||
| target={article().contents?.[0]?.url == null | ||
| ? "_blank" | ||
| : undefined} | ||
| article().language ?? undefined} | ||
| target="_blank" | ||
| on:mouseover={() => props.setHover?.(true)} | ||
| on:mouseout={() => props.setHover?.(false)} | ||
| data-llm-summary-label={t`Summarized by LLM`} | ||
| class="prose dark:prose-invert break-words overflow-wrap px-4 pb-4 before:content-[attr(data-llm-summary-label)] before:mr-1 before:text-sm before:bg-muted before:text-muted-foreground before:p-1 before:rounded-sm before:border" | ||
| classList={{ "before:border-transparent": !props.hover?.() }} | ||
| /> | ||
| )} | ||
| </Show> | ||
| <a | ||
| href={article().contents?.[0]?.url ?? article().url ?? | ||
| article().iri} | ||
| hreflang={article().contents?.[0]?.language ?? article().language ?? | ||
| undefined} | ||
| target={article().contents?.[0]?.url == null ? "_blank" : undefined} | ||
| on:mouseover={() => props.setHover?.(true)} | ||
| on:mouseout={() => props.setHover?.(false)} | ||
| class="block p-4 border-t bg-muted text-center group-last:rounded-b-xl" | ||
| classList={{ | ||
| "text-muted-foreground": !props.hover?.(), | ||
| "text-accent-foreground": props.hover?.(), | ||
| "border-t-muted": !props.hover?.(), | ||
| "dark:border-t-black": props.hover?.(), | ||
| }} | ||
| class="block p-4 border-t bg-muted text-center group-last:rounded-b-xl" | ||
| classList={{ | ||
| "text-muted-foreground": !props.hover?.(), | ||
| "text-accent-foreground": props.hover?.(), | ||
| "border-t-muted": !props.hover?.(), | ||
| "dark:border-t-black": props.hover?.(), | ||
| }} | ||
| > | ||
| {t`Read full article`} | ||
| </a> | ||
| } | ||
| > | ||
| {t`Read full article`} | ||
| </a> | ||
| <InternalLink | ||
| href={article().contents?.[0]?.url ?? article().url ?? | ||
| article().iri} | ||
| internalHref={`/@${article().actor.username}/${article().publishedYear}/${article().slug}`} | ||
| hreflang={article().contents?.[0]?.language ?? | ||
| article().language ?? undefined} | ||
| on:mouseover={() => props.setHover?.(true)} | ||
| on:mouseout={() => props.setHover?.(false)} | ||
| class="block p-4 border-t bg-muted text-center group-last:rounded-b-xl" | ||
| classList={{ | ||
| "text-muted-foreground": !props.hover?.(), | ||
| "text-accent-foreground": props.hover?.(), | ||
| "border-t-muted": !props.hover?.(), | ||
| "dark:border-t-black": props.hover?.(), | ||
| }} | ||
| > | ||
| {t`Read full article`} | ||
| </InternalLink> | ||
| </Show> |
There was a problem hiding this comment.
There's significant code duplication in how links to articles are rendered. The logic to switch between an internal link (<InternalLink>) for local articles and a standard external link (<a>) for remote articles is repeated for the title, summary, and "Read full article" button. This makes the component harder to read and maintain.
Consider creating a dedicated component, e.g., ArticleLink, that encapsulates this logic. This component would accept the article data and render the appropriate link type, simplifying the ArticleCardInternal component considerably.
For example:
function ArticleLink(props) {
const isLocal = () => props.article.actor.local;
// ... other logic
return (
<Show when={isLocal()} fallback={<a {...commonProps} target="_blank">{props.children}</a>}>
<InternalLink {...commonProps}>{props.children}</InternalLink>
</Show>
)
}Then you could use it like <ArticleLink article={article()} ...>{...}</ArticleLink> in all the places where this logic is duplicated.
Fill in 6 missing translations across ja-JP, ko-KR, zh-CN, and zh-TW for the new article edit page strings.
malkoG
force-pushed
the
feature/article-delete-web-next
branch
from
176583b to
a2613d7
Compare
dahlia
force-pushed
the
main
branch
2 times, most recently
from
ff82b29 to
38a6e99
Compare
1 participant
Summary
Wires up article deletion on the detail page and adds a proper 404 view for deleted/missing articles.
The
PostActionMenudelete button (which uses the existingdeletePostmutation) now passes anonDeletedcallback that navigates to the author's profile page after successful deletion. Visiting a deleted article's URL shows a "Page Not Found" message instead of a blank page.Stacks onto #223.