feat: explicitly hide articles #473
Conversation
WalkthroughAdds an isHidden boolean column to articles and articleCounts, updates multiple covering and unique indexes to include isHidden, modifies API queries to conditionally skip the isHidden=false filter when showHidden is present, and updates the build script to request articles with the showHidden flag. Changes
Sequence Diagram(s)sequenceDiagram
participant BuildScript as Build Routes Script
participant API as Blog BFF Articles API
participant DB as Database
BuildScript->>API: GET /articles?skip=..&take=..&showHidden
API->>API: Parse query -> showHidden present?
alt showHidden present
API->>DB: SELECT ... WHERE ... (no isHidden filter)
else showHidden absent
API->>DB: SELECT ... WHERE isHidden = false
end
DB-->>API: Rows + Count
API-->>BuildScript: JSON response
note over DB: Covering indexes updated to include isHidden
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (1)
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 |
|
PR is detected, will deploy to dev environment |
|
Deploy failed, please check the logs in jenkins for more details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/blog-bff/shared/schema/src/lib/schema.ts (1)
146-167: articleCounts table missing isHidden column.The
articleCountstable doesn't include anisHiddencolumn, but the API code atlibs/blog-bff/articles/api/src/lib/api.tsline 68 attempts to filter on it. This will cause runtime errors when the count query is executed without theshowHiddenparameter.Consider adding
isHiddento the articleCounts table or adjusting the count query logic to work without it:export const articleCounts = sqliteTable( 'article_counts', { lang: integer('lang').notNull(), status: integer('status').notNull(), + isHidden: integer('is_hidden', { mode: 'boolean' }).notNull(), isNews: integer('is_news', { mode: 'boolean' }).notNull(), isGuide: integer('is_guide', { mode: 'boolean' }).notNull(), isInDepth: integer('is_in_depth', { mode: 'boolean' }).notNull(), isRecommended: integer('is_recommended', { mode: 'boolean' }).notNull(), rowCount: integer('row_count').notNull(), }, (table) => [ unique().on( table.lang, table.status, + table.isHidden, table.isNews, table.isGuide, table.isInDepth, table.isRecommended, ), ], );
🧹 Nitpick comments (2)
libs/blog-bff/articles/api/src/lib/api.ts (1)
145-147: Consider requiring an explicit truthy value for showHidden parameter.The current logic
showHidden !== undefinedmeans any query string value (including empty string like?showHidden=or?showHidden=false) will include hidden articles. This might be unexpected behavior.Consider checking for an explicit truthy value:
-function showHiddenFilter(table: typeof articles, showHidden?: string) { - return showHidden !== undefined ? [] : [eq(table.isHidden, false)]; +function showHiddenFilter(table: typeof articles, showHidden?: string) { + return showHidden === 'true' || showHidden === '1' ? [] : [eq(table.isHidden, false)]; }apps/blog/scripts/build-routes.mjs (1)
50-50: Query parameter value inconsistency.The
showHiddenparameter is added without an explicit value (?showHidden), which will result in an empty string value in the API. The current API implementation treats any defined value (including empty string) as "show hidden", but this relies on implicit behavior.Consider using an explicit value for clarity:
- const url = `${API_BASE_URL}/articles?skip=${skip}&take=${take}&showHidden`; + const url = `${API_BASE_URL}/articles?skip=${skip}&take=${take}&showHidden=true`;This makes the intent clearer and aligns better if the API is updated to check for truthy values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/blog/scripts/build-routes.mjs(1 hunks)libs/blog-bff/articles/api/src/lib/api.ts(3 hunks)libs/blog-bff/shared/schema/src/lib/schema.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libs/blog-bff/articles/api/src/lib/api.ts (1)
libs/blog-bff/shared/schema/src/lib/schema.ts (1)
articles(52-144)
🔇 Additional comments (5)
libs/blog-bff/articles/api/src/lib/api.ts (2)
86-122: Single article endpoint bypasses isHidden filter.The
/:slugendpoint retrieves articles without checking theisHiddenflag, potentially exposing hidden articles to anyone who knows the slug. This might be intentional for preview purposes, but it creates an inconsistency with the listing endpoint.Verify whether hidden articles should be accessible via direct slug lookup, or if the endpoint should respect the
isHiddenflag (or require authentication/authorization to view hidden articles).
53-53: Filter logic works correctly for article listing.The spread operator correctly applies the hidden filter when
showHiddenis absent, maintaining backward compatibility while adding the new feature.libs/blog-bff/shared/schema/src/lib/schema.ts (2)
102-142: Excellent index coverage for isHidden filtering.All article-related covering indexes have been updated to include
isHidden, ensuring efficient query performance when filtering by hidden status. The placement ofisHiddenearly in the index (afterstatus) is optimal for selective filtering.
77-77: Confirm migration sets default value foris_hidden.
No SQL or TS migration was found for this column; ensure existing rows are backfilled withfalse.apps/blog/scripts/build-routes.mjs (1)
50-50: Verify build-time inclusion of hidden articles is intentional.The build script now fetches all articles including hidden ones (
showHiddenparameter present). This means hidden articles will be pre-rendered during the build process and accessible via direct URLs, even though they won't appear in listings.Confirm this is the intended behavior. If hidden articles should not be accessible at all (not even pre-rendered), consider:
- Removing the
showHiddenparameter here to exclude them from SSG- Adding runtime checks in the article component to handle hidden articles
- Or implementing authentication/authorization for accessing hidden articles
If the intention is to use hidden articles as "unlisted" (accessible by direct link but not in listings), the current implementation is correct.
|
PR is detected, will deploy to dev environment |
|
Deploy failed, please check the logs in jenkins for more details. |
DDonochVA
force-pushed
the
feat/explicit-hidden-articles
branch
from
d829590 to
a920e08
Compare
|
PR is detected, will deploy to dev environment |
|
Deploy failed, please check the logs in jenkins for more details. |
|
PR is detected, will deploy to dev environment |
|
Deploy failed, please check the logs in jenkins for more details. |
|
PR is detected, will deploy to dev environment |
1 similar comment
|
PR is detected, will deploy to dev environment |
|
Deploy failed, please check the logs in jenkins for more details. |
|
PR is detected, will deploy to dev environment |
|
Deploy failed, please check the logs in jenkins for more details. |
|
PR is detected, will deploy to dev environment |
|
Deploy failed, please check the logs in jenkins for more details. |
|
PR is detected, will deploy to dev environment |
|
Deploy failed, please check the logs in jenkins for more details. |
|
PR is detected, will deploy to dev environment |
|
Deploy failed, please check the logs in jenkins for more details. |
|
PR is detected, will deploy to dev environment |
|
Deploy failed, please check the logs in jenkins for more details. |
|
PR is detected, will deploy to dev environment |
4 similar comments
|
PR is detected, will deploy to dev environment |
|
PR is detected, will deploy to dev environment |
|
PR is detected, will deploy to dev environment |
|
PR is detected, will deploy to dev environment |
|
Deployed to dev environment |
Summary by CodeRabbit
New Features
Chores