docs(design): promote admin dashboard from proposed to partial#675
docs(design): promote admin dashboard from proposed to partial#675
Conversation
Per docs/design/README.md's lifecycle convention, the admin dashboard design doc is now "partial" — P1 (DynamoDB CRUD + AdminForward), P2 (S3 buckets), and P3 (React SPA) have all shipped on main, with P4 (operator docs in #674) the only remaining phase, and even then its TLS / role / CSRF deliverables are already live in P1. Renamed via git mv so the history follows. Added an "Implementation status" table at the top of the doc indexing each phase to the PRs that landed it, plus an explicit list of the outstanding open items (rolling-upgrade compat flag, S3 object browser, TLS cert hot-reload) so future readers know what is still owed against the original proposal.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 38 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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 |
|
@claude review |
|
Claude finished @bootjp's task in 2m 14s —— View job Code Review
Overall, this is a clean lifecycle promotion — Issues1. P2 marked ✅ shipped but #669 and #673 are still in flight (medium) The implementation status table reads:
But the PR description says:
Those two PRs aren't on main yet, so P2 write paths have not shipped. The table should either mark P2 as 🟡 (like P4) and note what's still pending, or split the row to distinguish what has landed (#658) from what is in flight (#669, #673). Leaving it as ✅ shipped is factually incorrect and will mislead future readers about the actual state of main. 2. P1 row claims criteria 1–6 shipped, but criterion 5 is explicitly listed as deferred (minor) The P1 row reads:
Section 8 of the doc itself defines the P1 DoD as: "Section 3.3.2 の受け入れ基準 1〜6 がすべてグリーンでない限り…マージしない". Yet the outstanding items section states that criterion 5 (rolling-upgrade compat flag) was deferred. The P1 cell implying criteria 1–6 all shipped is inconsistent with the explicit deferral. Suggest changing "acceptance criteria 1–6" to "acceptance criteria 1–4, 6 (criterion 5 deferred — see outstanding items)" to keep the table honest. Fix this → Cosmetic notes (not blocking)
What's right
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a5b65f26e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | Phase | Status | Landed via | | ||
| |---|---|---| | ||
| | **P1** — `internal/admin/` skeleton, auth, DynamoDB list/create/describe/delete, AdminForward (Section 3.3 acceptance criteria 1–6) | ✅ shipped | #634, #635, #644, #648 | | ||
| | **P2** — S3 bucket list/create/delete/ACL, DescribeTable | ✅ shipped | #658 (read-only slice 1), #669 (write paths slice 2a), #673 (AdminForward integration slice 2b) | |
There was a problem hiding this comment.
docs/design/README.md requires status to be assigned conservatively from code already on main, but this row marks P2 as ✅ shipped while citing #669/#673, which this change’s own context describes as still in flight. That makes the lifecycle table misleading for readers who rely on it to understand what is actually landed versus pending, so P2 should remain in-progress until those slices are merged.
Useful? React with 👍 / 👎.
![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 updates the elastickv Admin Dashboard design document to reflect its current "Partial" implementation status, detailing the progress of phases P1 through P4 and listing outstanding items. Feedback from the reviewer points out several inaccuracies in the status table, specifically regarding the completion of P1 and P2, and identifies a typo where DescribeTable should be DescribeBucket.
|
|
||
| | Phase | Status | Landed via | | ||
| |---|---|---| | ||
| | **P1** — `internal/admin/` skeleton, auth, DynamoDB list/create/describe/delete, AdminForward (Section 3.3 acceptance criteria 1–6) | ✅ shipped | #634, #635, #644, #648 | |
There was a problem hiding this comment.
The table indicates that P1 is fully shipped including acceptance criteria 1–6. However, criterion 5 (rolling-upgrade flag) is listed as deferred. According to repository rules, when a design document identifies a significant operational risk, such as the inability to perform rolling upgrades, it must also detail potential mitigation strategies, like implementing a temporary bridge or proxy mode. Please update the document to include these strategies.
| | **P1** — `internal/admin/` skeleton, auth, DynamoDB list/create/describe/delete, AdminForward (Section 3.3 acceptance criteria 1–6) | ✅ shipped | #634, #635, #644, #648 | | |
| | P1 — internal/admin/ skeleton, auth, DynamoDB list/create/describe/delete, AdminForward (Section 3.3 criteria 1–4, 6) | ✅ shipped | #634, #635, #644, #648 | |
References
- When a design document identifies a significant operational risk, such as the inability to perform rolling upgrades, it must also detail potential mitigation strategies, like implementing a temporary bridge or proxy mode.
| # elastickv Admin Dashboard Design | ||
|
|
||
| **Status:** Proposed | ||
| **Status:** Partial — P1, P2, and P3 of the implementation plan have shipped; P4 ships its operator documentation alongside this rename. |
There was a problem hiding this comment.
The summary status claims that P2 has shipped and that P4 documentation is shipping alongside this rename. However, the PR description notes that P2 write paths (#669, #673) and P4 documentation (#674) are still in flight. The summary should be updated to accurately reflect the current state of the main branch.
| **Status:** Partial — P1, P2, and P3 of the implementation plan have shipped; P4 ships its operator documentation alongside this rename. | |
| Status: Partial — P1 and P3 have shipped; P2 is partially live (read-only), and P4 (operator docs) is in flight via #674. |
| **Status:** Partial — P1, P2, and P3 of the implementation plan have shipped; P4 ships its operator documentation alongside this rename. | ||
| **Author:** bootjp | ||
| **Date:** 2026-04-24 | ||
| **Last updated:** 2026-04-26 (renamed from `_proposed_` to `_partial_` after P1 + P2 + P3 landed) |
There was a problem hiding this comment.
The update note states that P2 has landed, which contradicts the 'in flight' status of the S3 write paths. It should be clarified that only the read-only portion of P2 has landed.
| **Last updated:** 2026-04-26 (renamed from `_proposed_` to `_partial_` after P1 + P2 + P3 landed) | |
| Last updated: 2026-04-26 (renamed from proposed to partial after P1, P3, and P2-read-only landed) |
| | Phase | Status | Landed via | | ||
| |---|---|---| | ||
| | **P1** — `internal/admin/` skeleton, auth, DynamoDB list/create/describe/delete, AdminForward (Section 3.3 acceptance criteria 1–6) | ✅ shipped | #634, #635, #644, #648 | | ||
| | **P2** — S3 bucket list/create/delete/ACL, DescribeTable | ✅ shipped | #658 (read-only slice 1), #669 (write paths slice 2a), #673 (AdminForward integration slice 2b) | |
There was a problem hiding this comment.
The status for P2 is marked as 'shipped', but the implementation of S3 write paths (create/delete/ACL) is still in flight via #669 and #673. Additionally, DescribeTable appears to be a typo for DescribeBucket, as the DynamoDB equivalent was already covered in P1. This row should be updated to reflect the partial status and correct the typo.
| | **P2** — S3 bucket list/create/delete/ACL, DescribeTable | ✅ shipped | #658 (read-only slice 1), #669 (write paths slice 2a), #673 (AdminForward integration slice 2b) | | |
| | P2 — S3 bucket list/create/delete/ACL, DescribeBucket | 🟡 partial | #658 (read-only slice 1); #669 (write paths), #673 (AdminForward) in flight | |
Two findings from claude review #675: 1. **P2 was marked ✅ shipped but #669 / #673 are still in flight.** PR description correctly noted slices 2a/2b haven't merged, but the table cell read "✅ shipped". Future readers reading the table alone would conclude main has the S3 write paths today, which is wrong. Fixed: P2 is now 🟡 partial with the in-flight PRs explicitly called out, mirroring the P4 row's shape. 2. **P1 row claimed criteria 1–6 but criterion 5 is in the outstanding items as deferred.** The two cells contradicted each other. Updated P1 to "criteria 1–4 + 6 (criterion 5 deferred — see outstanding items)" so the table is consistent with the outstanding-items section below. The cosmetic notes (Last-updated field, verbose Status header) are left for a future cleanup; they are not factually wrong, only non-conforming to the README convention. Calling them out as a follow-up rather than baking the precedent into this doc.
|
@claude review 7617daf addresses both review findings:
Cosmetic notes (Last-updated field, Status verbosity) noted for a future cleanup — they're not factually wrong, just non-conforming to the README convention. |
|
Claude finished @bootjp's task in 47s —— View job Code Review (follow-up)
Review of 7617dafBoth targeted fixes landed correctly:
One remaining inconsistency that the fix commit didn't catch: Lines 3 and 6 still describe P2 as fully shipped. Line 3:
Line 6:
Both of these were not updated when P2 was demoted to 🟡 in the table. A reader who only skims the header will get the same incorrect impression the table now correctly avoids. Fix this → Everything else is in good shape. Once lines 3 and 6 are aligned with the table, the document is internally consistent and ready to merge. |
…ane label Three substantive findings + one nit from claude review on #674: 1. **HS256 key size (high)**: said "≥ 32 raw bytes / 44 base64 chars" but the validator in internal/admin/config.go enforces exactly 64 raw bytes. A 32-byte key fails startup with a confusing error. Updated to "exactly 64 raw bytes — 88 base64 chars (standard padding) or 86 (RawURLEncoding)". Operators copying the doc into key-generation tooling now produce a key that actually validates. 2. **Audit log sample (moderate)**: showed method=POST / path=... / status=201 / duration=8.2ms — none of those fields are emitted. The actual slog entries use operation= + a resource key (bucket= or table=). Replaced the example with two real shapes (leader-direct and forwarded), so an operator building a log parser against the documented contract finds matches. 3. **Cross-reference link (moderate)**: line 249 used the _partial_ filename for the design doc, but #675 (the rename PR) hasnt landed yet. Point to _proposed_ (matches main today) with an inline note that the link will follow once #675 lands. The intro paragraph at line 5 already used _proposed_, so this removes the inconsistency claude flagged. 4. **"data-plane" label nit**: /admin/api/v1/cluster is an admin- plane endpoint, not data-plane. "data-plane" implied the DynamoDB / S3 / Redis ports. Replaced with "admin". The cosmetic Last-updated / Status verbosity items on #675 are tracked separately.
…ane label Three substantive findings + one nit from claude review on #674: 1. **HS256 key size (high)**: said "≥ 32 raw bytes / 44 base64 chars" but the validator in internal/admin/config.go enforces exactly 64 raw bytes. A 32-byte key fails startup with a confusing error. Updated to "exactly 64 raw bytes — 88 base64 chars (standard padding) or 86 (RawURLEncoding)". Operators copying the doc into key-generation tooling now produce a key that actually validates. 2. **Audit log sample (moderate)**: showed method=POST / path=... / status=201 / duration=8.2ms — none of those fields are emitted. The actual slog entries use operation= + a resource key (bucket= or table=). Replaced the example with two real shapes (leader-direct and forwarded), so an operator building a log parser against the documented contract finds matches. 3. **Cross-reference link (moderate)**: line 249 used the _partial_ filename for the design doc, but #675 (the rename PR) hasnt landed yet. Point to _proposed_ (matches main today) with an inline note that the link will follow once #675 lands. The intro paragraph at line 5 already used _proposed_, so this removes the inconsistency claude flagged. 4. **"data-plane" label nit**: /admin/api/v1/cluster is an admin- plane endpoint, not data-plane. "data-plane" implied the DynamoDB / S3 / Redis ports. Replaced with "admin". The cosmetic Last-updated / Status verbosity items on #675 are tracked separately.
Latest claude review on #675 flagged that lines 3 and 6 still described P2 as fully shipped: Line 3: "P1, **P2**, and P3 of the implementation plan have shipped" Line 6: "after P1 + **P2** + P3 landed" But the status table I added in 7617daf correctly marks P2 as 🟡 partial (read-only slice 1 in #658 landed; write paths #669 and #673 still in flight). The header prose contradicting the table is exactly the kind of inconsistency that triggers the rename to `_partial_` in the first place — readers skimming the top of the doc would conclude P2 was done. Rewrote the Status line and the parenthetical on Last updated to match the table, calling out P1 + P3 as fully shipped, P2 as the read-only slice only, and P4 as TLS/role/CSRF live with the operator doc landing now. Pointed readers at the per-phase status table for the breakdown. No other changes; the table itself was already correct.
|
@claude review Aligned the header prose with the P2 partial status table:
Status table itself was already correct; this is prose-only. Latest commit: 25ee3dc. |
|
Claude finished @bootjp's task in 1m 34s —— View job Code Review (follow-up on 25ee3dc)
Review of 25ee3dcThe prose-only fix resolves the last remaining inconsistency flagged in the previous round. Line 3 (Status header): Before: ✅ Now accurately reflects main. The "See the status table below for the per-phase breakdown" pointer is a nice touch. Line 6 (Last updated parenthetical): Before: ✅ Consistent with line 3 and the table. Full document consistency checkRunning through all phase references across the doc:
The What's right
No remaining issues. This is ready to merge. |
…ane label Three substantive findings + one nit from claude review on #674: 1. **HS256 key size (high)**: said "≥ 32 raw bytes / 44 base64 chars" but the validator in internal/admin/config.go enforces exactly 64 raw bytes. A 32-byte key fails startup with a confusing error. Updated to "exactly 64 raw bytes — 88 base64 chars (standard padding) or 86 (RawURLEncoding)". Operators copying the doc into key-generation tooling now produce a key that actually validates. 2. **Audit log sample (moderate)**: showed method=POST / path=... / status=201 / duration=8.2ms — none of those fields are emitted. The actual slog entries use operation= + a resource key (bucket= or table=). Replaced the example with two real shapes (leader-direct and forwarded), so an operator building a log parser against the documented contract finds matches. 3. **Cross-reference link (moderate)**: line 249 used the _partial_ filename for the design doc, but #675 (the rename PR) hasnt landed yet. Point to _proposed_ (matches main today) with an inline note that the link will follow once #675 lands. The intro paragraph at line 5 already used _proposed_, so this removes the inconsistency claude flagged. 4. **"data-plane" label nit**: /admin/api/v1/cluster is an admin- plane endpoint, not data-plane. "data-plane" implied the DynamoDB / S3 / Redis ports. Replaced with "admin". The cosmetic Last-updated / Status verbosity items on #675 are tracked separately.
…ane label Three substantive findings + one nit from claude review on #674: 1. **HS256 key size (high)**: said "≥ 32 raw bytes / 44 base64 chars" but the validator in internal/admin/config.go enforces exactly 64 raw bytes. A 32-byte key fails startup with a confusing error. Updated to "exactly 64 raw bytes — 88 base64 chars (standard padding) or 86 (RawURLEncoding)". Operators copying the doc into key-generation tooling now produce a key that actually validates. 2. **Audit log sample (moderate)**: showed method=POST / path=... / status=201 / duration=8.2ms — none of those fields are emitted. The actual slog entries use operation= + a resource key (bucket= or table=). Replaced the example with two real shapes (leader-direct and forwarded), so an operator building a log parser against the documented contract finds matches. 3. **Cross-reference link (moderate)**: line 249 used the _partial_ filename for the design doc, but #675 (the rename PR) hasnt landed yet. Point to _proposed_ (matches main today) with an inline note that the link will follow once #675 lands. The intro paragraph at line 5 already used _proposed_, so this removes the inconsistency claude flagged. 4. **"data-plane" label nit**: /admin/api/v1/cluster is an admin- plane endpoint, not data-plane. "data-plane" implied the DynamoDB / S3 / Redis ports. Replaced with "admin". The cosmetic Last-updated / Status verbosity items on #675 are tracked separately.
1 participant
Per
docs/design/README.md's lifecycle convention, the admin dashboard design doc is now "partial":Independent of the in-flight slice 2 PRs (#669/#673) and the docs PR (#674) — this rename only reflects what is already on main today, plus an "Implementation status" table mapping each phase to the PR it landed in.
What this PR changes
git mvthe design doc from2026_04_24_proposed_admin_dashboard.mdto2026_04_24_partial_admin_dashboard.mdso its history followsWhen the rolling-upgrade flag lands, the doc gets renamed once more to
2026_04_24_implemented_admin_dashboard.mdper the README's lifecycle convention.